Repository navigation
fix: evaluate a NULL-guarded argument for every row when only shared parts of it can fail - #7590
Draft
dwsmith1983 wants to merge 120 commits into
Draft
dwsmith1983 wants to merge 120 commits into
dwsmith1983 wants to merge 120 commits into
Conversation
…ull-guarded children Map lookups with float, collated or complex keys were declined by the serde without a dispatch fallback, so the whole projection fell back to Spark. Four array and map serdes reproduce NULL propagation with a guard that serializes the child twice, so a stateful child was evaluated twice natively and returned wrong answers. Mix the codegen dispatch fallback into the map lookups, and decline any nondeterministic child in size, array_append, arrays_zip and map_from_arrays through one shared gate so Spark's generated code evaluates it once. Closes apache#5580 Closes apache#5781
…d-nondeterministic-children
…in the dispatched fixtures
…e guard filters its evaluation
…e arrays_zip types With ANSI on, an item that raises on a row whose array is NULL raises in Spark but not on the native path, because the NULL guard skips the item there. The serde now reports that case as incompatible when the array is nullable, so it runs through the JVM codegen dispatcher by default and the native guard stays behind allowIncompatible. The divergence is recorded in the compatibility guide with its tracking issue, and a fixture pins the raise, the dispatch and the native path for a non-nullable array. The shared nondeterministic reason is now a full sentence for the generated guide, the arrays_zip type bullet names the element types the native kernel declines, and the arrays_zip fixture gets its comments in the right order plus a dispatched query over an array column.
…d-nondeterministic-children
An unsuffixed -0.0 is a decimal literal with no signed zero, so CAST(-0.0 AS DOUBLE) looked up +0.0 and found the key on the native path too. The lookups now use -0.0D and CAST(-0.0D AS FLOAT), which keep the sign, so the fixtures fail on results when the float key decline is missing. The contributor guide names the literal form as well.
…d-nondeterministic-children
…d-nondeterministic-children
…d-nondeterministic-children
…d-nondeterministic-children
…d-nondeterministic-children
…d-nondeterministic-children
…lacement can be NULL The generated code of a three-argument array_join evaluates the replacement first and skips the array and delimiter when it is NULL. The default rule only treated children after the first as skippable, so a subexpression shared through the array was missed: a dispatched lookup could return NULL where Spark, which evaluates the shared subexpression for every row, raises.
…d-nondeterministic-children
…d-nondeterministic-children
…d-nondeterministic-children
…the dispatch gates With the item now evaluated natively on rows whose array is NULL, the array_append item gate is dropped. array_append keeps the rule of declining a nondeterministic operand only over a nullable array, and that case now goes to the JVM codegen dispatcher. The calendar-interval and shared-subexpression declines move into one GatedCodegenDispatchFallback trait that the six dispatching serdes mix in. A shared subexpression only counts in the dividend of a division or pmod, since Spark always evaluates the divisor. Fixtures fold the duplicated array_append files into array_append_eager_eval.sql, replace the extra nonzero tables with a divisor that cannot be zero, and move the folded-literal dispatch tests from CometMapExpressionSuite to SQL fixtures.
…d-nondeterministic-children
…ures Drop comments that restate the code, a query or its expectation, and shorten the rest to the Spark codegen behavior and fixture values they explain.
…d-nondeterministic-children
…d-nondeterministic-children # Conflicts: # spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala # spark/src/main/scala/org/apache/spark/sql/comet/execution/shuffle/CometShuffleExchangeExec.scala
The serde wraps decimal arithmetic in CheckOverflow before the dispatcher sees it, so the decline only recognizes a shared decimal key when both sides are promoted. Without that, Comet returned rows where Spark raises DIVIDE_BY_ZERO on a NULL map.
…d-nondeterministic-children
…d-nondeterministic-children
…d-nondeterministic-children
…d-nondeterministic-children
…d-nondeterministic-children
…ng depends on the plan Whether Spark evaluates a shared subexpression for every row depends on the operator. A projection, an aggregate's function inputs, shuffle keys, the TopK projection and a sort's first key share it as the setting says. A filter, a join, an aggregate's results and other operators share it only in some Spark versions or without whole-stage codegen. In those operators a NULL-guarded lookup whose skipped argument holds a shared subexpression now falls back to Spark instead of evaluating it for every row, which could raise where Spark returns rows.
…not fail Skipping a shared subexpression on a NULL row only changes the result when evaluating it would raise. Count only shared subexpressions that can fail, using a conservative allowlist that follows the eval mode: comparisons, NULL checks, boolean logic, constructors, string case conversion, trim, substring, length, hashes and date parts, plus non-ANSI arithmetic and casts, widening casts other than to a timestamp, and casts of a literal that evaluate. Only the arguments after the first nullable one can be skipped. The codegen dispatcher's check uses the same rule, so these shapes stay native.
…parts of it can fail With ANSI on, Spark evaluates a subexpression its operator shares for every row, before any NULL check, so it raises on a row whose array or map is NULL. Native element_at and map_from_arrays skipped the argument on those rows and returned results where Spark fails. Where a skipped argument holds a shared subexpression that can fail, the NULL-guarded lookups now evaluate it for every row if everything else in the argument cannot fail and the operator's sharing is certain, as Spark does. Otherwise they fall back to Spark. map_from_arrays has no form that evaluates its values for every row, so it falls back in that case. Outside an operator scope, element_at and map_from_arrays keep their guards. Closes apache#6970.
…rd-shared-subexpr
…rd-shared-subexpr
This was referenced Nov 10, 2026
Draft
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
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.
Which issue does this PR close?
Closes #6970. This is the last of three pull requests for it.
These build on #5867, which adds the shared-subexpression check they extend, so #5867 merges first. Order after it:
They target
main, so until #5867 merges each one also shows its commits, and the ones before it.Rationale for this change
With ANSI on, Spark evaluates a subexpression its operator shares for every row, before any NULL check, so it raises on a row whose array or map is NULL. Native
element_atandmap_from_arraysskipped the argument on those rows and returned rows where Spark fails.What changes are included in this PR?
Where an argument a NULL guard would skip holds a shared subexpression that can fail,
element_at,GetArrayItem,GetMapValueand the otherwithNullShortCircuitcallers now:map_from_arrayshas no native form that evaluates its values for every row, since nativemaptakes NULL rows from the keys alone, so it falls back in that case. Outside an operator's scope,element_atandmap_from_arrayskeep their guards.How are these changes tested?
Three new files cover
element_aton arrays and maps andmap_from_arrays, in a projection, an aggregate and a filter, with elimination on and off, including the issue's repro. The error queries fail on #7589, where Comet returned rows. Unit tests cover the eager check and the guards outside a scope. The suites and TPC-DS plan stability pass on Spark 3.5 and 4.1, with no plan changes, and the new files pass on 4.2.