andygrove commented on code in PR #5607:
URL: https://github.com/apache/datafusion-comet/pull/5607#discussion_r3904707815
##########
spark/src/main/scala/org/apache/comet/serde/strings.scala:
##########
@@ -82,7 +82,10 @@ object CometUpper extends
CometCaseConversionBase[Upper]("upper")
object CometLower extends CometCaseConversionBase[Lower]("lower")
-object CometLength extends CometScalarFunction[Length]("length") {
+object CometLength extends CometScalarFunction[Length]("length") with
CodegenDispatchFallback {
Review Comment:
Thanks @sunchao, I ran this down and the trace holds. Reproduced on this
branch (Spark 4.1, ANSI on):
```sql
CREATE TABLE t (flag BOOLEAN, n INT) USING parquet;
INSERT INTO t VALUES (true, NULL), (false, NULL);
SELECT IF(flag, length(substring(X'00', CAST(1L DIV 0L AS INT), n)), 0) FROM
t;
```
Spark raises `[DIVIDE_BY_ZERO]`, Comet returns a row. Same for `bit_length`
and `octet_length`.
For anyone reading later, the three pieces that have to line up:
* `ConstantFolding` refuses to fold `1L DIV 0L` because it sits under an
`If` branch (it tags `FAILED_TO_EVALUATE` and leaves the node alone), so the
throwing literal survives into the physical plan.
* `TernaryExpression.nullSafeCodeGen` emits `Substring`'s `pos` code before
it tests `len`'s null, so Spark evaluates the division even though `n` is NULL.
* `Length`, `Substring`, `Cast` and `IntegralDivide` are all null-intolerant
and the dispatched tree reads exactly one ordinal, so `canShortCircuitNulls`
takes its single-ordinal branch and the kernel writes NULL before `ev.code`
runs.
One correction on scope: this isn't introduced here, it's the residual hole
in #5219. The single-ordinal branch assumes "there is nothing left for Spark to
evaluate ahead of that ordinal's own null check", and that's false whenever the
tree carries a literal-only subtree that throws. `upper` reproduces it on main
today, unchanged by this PR:
```sql
SELECT IF(flag, upper(substring('abc', CAST(1L DIV 0L AS INT), n)), NULL)
FROM t;
```
I confirmed that one on the same build: Spark raises, Comet doesn't.
So I'd rather fix `canShortCircuitNulls` than special-case the three length
serdes, otherwise we paper over three of the ~70 expressions that share the
hole. Filed as #5608, with the suggested guard and a regression test covering
`upper` plus all three roots from this PR.
@adibmbrk I don't think this needs to block the PR. Please add a link to
#5608 in the PR description so the connection isn't lost.
--
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]
