andygrove opened a new issue, #5218:
URL: https://github.com/apache/datafusion-comet/issues/5218
## Describe the bug
Three related defects in the JVM codegen dispatcher
(`spark/src/main/scala/org/apache/comet/codegen/`). The first produces wrong
results today; the other two are latent gaps in the same code where the
plan-time gate accepts a type the runtime dispatcher rejects.
Context: the dispatcher is not only used for `ScalaUDF`. Roughly 70 built-in
expressions route through `CometCodegenDispatch` (math, arrays, maps, strings,
json, csv, xpath, datetime), so the blast radius is wide.
### Finding 1 (confirmed): whole-tree null short-circuit suppresses runtime
errors that Spark raises
`CometBatchKernelCodegen.defaultBody` applies a "any input null implies null
output" short-circuit when every node in the bound tree is `NullIntolerant`. It
emits a pre-check on the **union of all input ordinals**:
```java
if (this.col0.isNull(i) || this.col1.isNullAt(i)) { output.setNull(i); }
else { ev.code; write }
```
Spark's own null handling is **per-node and left-to-right**:
`BinaryExpression.nullSafeCodeGen` emits the left child's code unconditionally,
then tests the left child's null, then the right child's. So when a left
subtree raises for a given row and a *different* input is null on that same
row, Spark raises but Comet silently returns NULL.
This is an error-semantics divergence only; values are unaffected. A
`NullIntolerant` node that ignored one of its inputs would already be a Spark
bug (checked `nanvl` specifically, which ignores its second argument when the
first is non-NaN, and it is correctly *not* `NullIntolerant`).
The divergence needs ANSI mode to be observable in most cases, and ANSI is
on by default in Spark 4. Many dispatch roots are `NullIntolerant` and have
error paths: `AddMonths`, `MonthsBetween`, `MakeTimestamp`, `GetTimestamp`,
`MakeDTInterval`, `ToNumber`, `Conv`, `WidthBucket`, `Pmod`, and others.
### Finding 2 (latent): plan-time gate accepts TIME, runtime dispatcher
rejects it
`CometBatchKernelCodegen.isSupportedDataType` accepts time types,
`primitiveArrowClasses` includes `TimeNanoVector`, and `emitTypedGetters` emits
a `getLong` case for it, but `CometScalaUDFCodegen.specFor` omits
`TimeNanoVector` and so falls into `case other => throw new
UnsupportedOperationException`.
Because `canHandle` greenlights the expression at plan time, there is no
fallback: the query fails at execute time. Not reachable today because Spark
4.1.3 rejects TIME columns in file-based data sources (`UNSUPPORTED_TIME_TYPE`
from `DataSourceUtils.verifySchema`), so this is a latent trap that goes live
as soon as Spark supports TIME in Parquet or a native operator produces a TIME
column.
### Finding 3 (latent): same TIME hole in the generic getter dispatch
`CometSpecializedGettersDispatch.get` has no time-type branch, so
`get(ordinal, TimeType)` throws. `emitSpecializedGetterExpr` and
`elementGetterCall` both handle time types, so the hole is only in the generic
path, which is exactly the path `CodegenFallback.eval(row)` and
`SafeProjection` (for struct/array arguments) use.
## Steps to reproduce
For Finding 1, with the default profile (Spark 4.1) and
`spark.sql.ansi.enabled=true`:
```sql
CREATE TABLE t (s STRING, i INT) USING parquet;
INSERT INTO t VALUES ('notadate', NULL);
SELECT add_months(CAST(s AS DATE), i) FROM t;
```
Result:
- Spark (`spark.comet.enabled=false`): throws `SparkDateTimeException`
`[CAST_INVALID_INPUT]`
- Comet: returns `NULL`
The plan confirms the dispatcher runs:
```
*(1) CometColumnarToRow
+- CometProject [add_months(CAST(s AS DATE), i)#21], [add_months(cast(s#8 as
date), i#9) AS ...]
+- CometNativeScan parquet spark_catalog.default.t[s#8,i#9] ...
```
`add_months` is the witness because it is a plain `BinaryExpression` whose
`doGenCode` goes through `defineCodeGen`/`nullSafeCodeGen`. Note that `pmod`
and the `div`/`mod` family are *not* witnesses: `DivModLike` deliberately
evaluates its right child first so it can skip the left when the divisor is
zero, so Spark also returns NULL there.
Also reproducible at the kernel level without a query plan, by compiling
`Add(Cast(BoundReference(0, StringType), IntegerType, EvalMode.ANSI),
BoundReference(1, IntegerType))` and running one batch of `('abc', NULL)`: the
kernel returns NULL while `expr.eval(row)` throws `SparkNumberFormatException`.
For Findings 2 and 3, no end-to-end reproducer exists yet (see above); they
are visible by inspection of the type surface accepted by `canHandle` versus
the one handled by `specFor` and `CometSpecializedGettersDispatch.get`.
## Expected behavior
Finding 1: Comet should raise the same error Spark raises. Proposed fix is
to restrict the short-circuit to trees with a **single distinct**
`BoundReference` ordinal. With one input, Spark also evaluates nothing when
that input is null, so the optimization stays exact, and the common unary
shapes (`upper`, `length`, `date_format`) keep the fast path. Multi-input trees
fall back to the plain `ev.code` plus `ev.isNull` body.
Findings 2 and 3: `specFor` should recognize `TimeNanoVector` and
`CometSpecializedGettersDispatch.get` should have a time-type branch, so the
runtime type surface matches what `canHandle` promises.
## Additional context
Found during an audit sweep of the codegen area. Existing coverage is
otherwise healthy: `CometCodegenSuite`, `CometCodegenSourceSuite`, and
`CometCodegenHOFSuite` pass 135/135 on the default profile, so these are gaps
rather than regressions.
Two further items were found in the same sweep and are *not* included here,
since they need separate discussion:
- Output decimal writes in `CometBatchKernelCodegenOutput` do not normalize
scale before writing, whereas Spark's `UnsafeRowWriter.write(ordinal, Decimal,
precision, scale)` calls `changePrecision` first and nulls out on failure. No
live reproducer was found because `CatalystTypeConverters.DecimalConverter`
already normalizes for `ScalaUDF`.
- The per-row body is inlined in `process`'s loop and never split (already
noted as `TODO(method-size)` in the source), so a deep tree can exceed Janino's
64KB method limit at execute time. `canHandle` gates on
`spark.sql.codegen.maxFields`, which bounds field count rather than code size.
--
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]
