Skip to content

fix: skip codegen dispatcher null short-circuit when a foldable subtree can raise - #5623

Merged
andygrove merged 2 commits into
apache:mainfrom
andygrove:fix-5608-shortcircuit-foldable
Sep 2, 2026
Merged

fix: skip codegen dispatcher null short-circuit when a foldable subtree can raise#5623
andygrove merged 2 commits into
apache:mainfrom
andygrove:fix-5608-shortcircuit-foldable

Conversation

@andygrove

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #5608.

Rationale for this change

CometBatchKernelCodegen.canShortCircuitNulls allows the pre-ev.code null short-circuit whenever the dispatched tree reads exactly one input ordinal, on the reasoning that a single ordinal leaves Spark nothing to evaluate ahead of that ordinal's own null check.

That is not true. A literal-only subtree between the root and the ordinal can still raise. ConstantFolding normally folds such a subtree away, but it deliberately leaves it in place when evaluating it throws and it sits inside a conditional branch (it tags the node FAILED_TO_EVALUATE and moves on), so the throwing expression survives into the physical plan. The kernel then writes NULL before ev.code runs, and an ANSI error Spark raises is silently swallowed:

CREATE TABLE t (flag BOOLEAN, n INT) USING parquet;
INSERT INTO t VALUES (true, NULL), (false, NULL);

SELECT IF(flag, upper(substring('abc', CAST(1L DIV 0L AS INT), n)), NULL) FROM t;

Spark raises [DIVIDE_BY_ZERO]; Comet returned a row. Three ordinary things line up: ConstantFolding refuses to fold 1L DIV 0L under the If branch, TernaryExpression.nullSafeCodeGen emits Substring's pos code before it tests len's null, and Upper / Substring / Cast / IntegralDivide are all null-intolerant over a single ordinal.

This is the residual hole in #5218: that fix added rootChildrenAreLeaves for the multi-ordinal case but left the single-ordinal branch unguarded. upper is just a convenient witness; any dispatched null-intolerant root with a throwing foldable subtree between it and its single input reproduces it.

What changes are included in this PR?

Adds a noSurvivingFoldableSubtree condition to canShortCircuitNulls: no node in the tree other than a Literal may be foldable. By the time the dispatcher sees the tree ConstantFolding has already run, so a surviving foldable non-Literal node is precisely one that threw during folding, which is exactly the dangerous case.

Literals are exempt, so the existing fast paths are untouched: none of upper(substring(s, 1, 2)), pmod(a, b), a + b, conv(a, b, c) or make_timestamp(...) contains a foldable non-Literal node. The scaladoc on canShortCircuitNulls is updated to record the new condition.

How are these changes tested?

  • CometCodegenSuite: a new end-to-end test asserting Comet raises the same DIVIDE_BY_ZERO Spark does for the query above, and that the codegen dispatcher actually ran for it. Verified to fail on main (cometErr.isDefined was false) and pass with the fix, on the spark-3.4, spark-3.5, spark-4.0 and spark-4.1 profiles.
  • CometCodegenSourceSuite: two generated-source tests, one asserting the short-circuit is not emitted for a single-ordinal tree carrying a throwing foldable subtree, and a counterpart asserting it is still emitted when the only foldable nodes are Literals, so the fix is not over-corrected.
  • Full CometCodegenSuite, CometCodegenSourceSuite, CometCodegenFuzzSuite and CometSpecializedGettersDispatchSuite pass (177 tests).

The length / bit_length / octet_length witnesses listed in the issue are left out here because they only apply once #5607 lands.

…ee can raise

The single-ordinal branch of `canShortCircuitNulls` assumed one input ordinal
leaves Spark nothing to evaluate ahead of that ordinal's null check. A foldable
subtree between the root and the ordinal breaks that assumption: `ConstantFolding`
leaves such a subtree in place when evaluating it throws and it sits inside a
conditional branch, so the throwing expression survives into the physical plan.

Require that no node other than a `Literal` is foldable.
@andygrove
andygrove requested a review from mbutrovich September 1, 2026 18:39
Assert foldability on the throwing node itself rather than on a path that
resolved to the string literal, where the check was vacuous. Trim the
`noSurvivingFoldableSubtree` scaladoc to sibling length: the worked witness
lives in the tests, and the rationale is already stated on
`canShortCircuitNulls`.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed ae5eb02ec73c326115b9bb73a34bbbc5ee5a6c78 against 1e10eedd6e0303adcac4573f44d5c07ffb0fbacd. No new P1/P2 findings. The guard preserves evaluation of surviving foldable subtrees before an input's null check while retaining the literal-only fast path. The regression covers conditional constant folding and verifies dispatcher use.

This was a source review, with no local test execution. Current CI has 63 successful checks, 9 skipped and 1 failed Spark 4.1 SQL core shard. I inspected the failed job log: SQLAppStatusListenerMemoryLeakSuite's no memory leak test fails because statusStore.listener.get.noLiveData() is false at SQLAppStatusListenerSuite.scala:1123. ScalaTest reports 12,851 tests passed and 1 failed. That failure remains unresolved, and I have not established whether it is related to this PR.

@andygrove
andygrove merged commit 10537e1 into apache:main Sep 2, 2026
142 of 143 checks passed
@andygrove
andygrove deleted the fix-5608-shortcircuit-foldable branch September 2, 2026 20:46
@andygrove

Copy link
Copy Markdown
Member Author

Merged. Thanks @sunchao

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Codegen dispatcher single-ordinal null short-circuit swallows ANSI errors from literal subtrees

2 participants