andygrove opened a new pull request, #5623:
URL: https://github.com/apache/datafusion-comet/pull/5623
## 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:
```sql
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.
`Literal`s 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 `Literal`s, 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
