[GLUTEN-10134][Vl] elt ansi function overlay - #12873
Draft
zhouyuan wants to merge 3 commits into
Draft
Conversation
Gluten has no per-function support for Spark's ANSI mode yet. Spark's elt raises an error on an out-of-range index when ANSI mode is on and returns NULL otherwise, and Velox has no elt at all, so the expression always falls back today. Implement elt in Gluten's function overlay instead of waiting for it to land in Velox. It is a vector function covering both elt(integer, varchar...) and elt(integer, varbinary...): a NULL index or a NULL selected input gives NULL, NULLs in inputs that are not selected are ignored, and an index outside [1, number of inputs] gives NULL with ANSI mode off and a user error with it on. ANSI mode is read from SparkQueryConfig::ansiEnabled(), which Gluten already populates from the session's spark.sql.ansi.enabled, the same channel Velox's own sparksql functions use. A vector function is used rather than a simple function because Velox's SimpleFunctionAdapter has no VectorReader<Variadic<T>> constructor on the initialize() path, so a variadic simple function cannot read the query config. Rows are grouped by the input they select and each input is then copied in one pass, which lets the result share the input's string buffers. Spark captures the ANSI decision in Elt.failOnError at analysis time while Velox derives it from the session config, so fall back when the two disagree, which can happen if spark.sql.ansi.enabled changes between analysis and execution. Note that docs/velox-backend-scalar-function-support.md is generated by tools/scripts/gen-function-support-docs.py from a test run and still lists elt as unsupported; it needs a regeneration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Velox's conv always lets the base conversion overflow, which its own comment
calls "consistent with Spark". That is only true with ANSI mode off: Spark's
NumberConverter.encode() saturates to 2^64 - 1 in that case, but raises
overflowInConvError when ANSI mode is on. conv is already reported as fully
supported, so an ANSI query silently gets the saturated value today.
Add an overlay override that keeps delegating the conversion to Velox's conv
and only adds the missing error. Before delegating, and only when ANSI mode is
on, it parses the digits the same way Velox does and raises a user error when
they do not fit in an unsigned 64-bit integer. That is exactly Spark's overflow
condition: the two checks in NumberConverter.encode() together detect that
accumulating the next digit would pass 2^64 - 1, which is what
std::from_chars reports as result_out_of_range.
Delegating rather than copying keeps the conversion logic, including its
handling of negative inputs and negative target bases, in one place, so the
overlay does not have to be kept in sync with fixes to Velox's conv. The sign
is applied after the digits are accumulated, so inputs like conv('-1', 10, 16)
still wrap around instead of raising an error, and an invalid base or an empty
input still gives NULL in ANSI mode.
As for elt, fall back when Conv.ansiEnabled, captured at analysis time,
disagrees with the session's spark.sql.ansi.enabled that Velox reads.
Note that Velox's conv only skips leading spaces while Spark trims the input
first, so an input led by another whitespace character, such as a tab, is
parsed as 0 by Velox. That is a pre-existing difference unrelated to ANSI mode
and is left alone here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…I-dependent legacySizeOfNull element_at over an array raises an error for an index past either end of the array when ANSI mode is on, and returns NULL otherwise. Velox's element_at is Presto's SubscriptImpl with allowOutOfBound fixed to true, so it always returns NULL. Add an overlay override that picks the instantiation matching the session's ANSI mode. The two remaining behaviors need no change and are kept: an index of 0 is an error whatever the ANSI mode is, which Velox already reports as "SQL array indices start at 1", and a key a map does not contain gives NULL, because the map side of SubscriptImpl does not look at allowOutOfBound and Spark's ElementAt does not pass failOnError to its map branch. GetMapValue, which Gluten also lowers to element_at, has had no failOnError since Spark 3.4, so it is unaffected too. On the Scala side, fall back for an array input when ElementAt.failOnError disagrees with the session's spark.sql.ansi.enabled, as for elt and conv, and also when defaultValueOutOfBound is set, since Velox has no way to return a default instead of NULL. Size needs no code change: Spark's legacySizeOfNull is 'spark.sql.legacy.sizeOfNull AND NOT ANSI mode', evaluated at analysis time, and ExpressionConverter already forwards Size.legacySizeOfNull to Velox's size(collection, legacySizeOfNull) as a literal, so the ANSI-dependent value is carried by the plan rather than re-derived natively. Add the tests that were missing to lock that in, over both values of spark.sql.legacy.sizeOfNull and both ANSI modes. Note that GetArrayItem, the 0-based array[i] operator lowered to Velox's get, has the same gap as element_at had: it throws invalidArrayIndexError under ANSI mode for an out-of-bound or negative index, while Velox's get returns NULL. It needs both allowOutOfBound and allowNegativeIndices flipped, and is left for a follow-up. The ANSI-on test suite added for elt is renamed to ScalarFunctionsValidateSuiteAnsiOn, matching MathFunctionsValidateSuiteAnsiOn, so it can host the ANSI counterparts of ScalarFunctionsValidateSuite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
zhouyuan
changed the title
|
Run Gluten Clickhouse CI on x86 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
fix ansi behavior section 4 in #10134
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?