[I] Codegen dispatcher: whole-tree NullIntolerant short-circuit suppresses ANSI errors, plus TIME type gaps between canHandle and the runtime dispatcher [datafusion-comet]

["andygrove (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@datafusion.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [I] Codegen dispatcher: whole-tree NullIntolerant short-circuit suppresses ANSI errors, plus TIME type gaps between canHandle and the runtime dispatcher [datafusion-comet]

["andygrove (via GitHub)" <gi...@apache.org>]

andygrove closed issue #5218: Codegen dispatcher: whole-tree NullIntolerant short-circuit suppresses ANSI errors, plus TIME type gaps between canHandle and the runtime dispatcher
URL: https://github.com/apache/datafusion-comet/issues/5218


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org