grorge123 opened a new pull request, #5526:
URL: https://github.com/apache/datafusion-comet/pull/5526
## Which issue does this PR close?
Closes #5525.
## Rationale for this change
Untyped constructors such as `map()`, `map('a', NULL)` and `array()` leave
`NullType` children in their output type. `CometBatchKernelCodegen.canHandle`
applied the same `isSupportedDataType` predicate to the output type and to
every `BoundReference` input, and that predicate had no `NullType` case, so any
expression whose output type contained `NullType` fell back to Spark and took
the whole operator with it.
Only the input side needs that restriction: `CometScalaUDFCodegen.specFor`
cannot build an `ArrowColumnSpec` for a `NullVector`, so a `NullType` input
would throw at execute time, after the point where a fallback is still
possible. On the output side the kernel just has to emit an all-null Arrow
`NullVector`, which serde (`NullType` type id) and `Utils.toArrowField`
(`ArrowType.Null`) already understand. No native changes are needed.
## What changes are included in this PR?
- `CometBatchKernelCodegen`: `isSupportedDataType` gains a private
`allowNullType` overload that recurses through array / struct / map children.
`canHandle` checks the output type with `allowNullType = true` and keeps
checking `BoundReference` inputs with the unchanged public predicate, so
`NullType` inputs still fall back at plan time. The doc comment states the
asymmetry and why.
- `CometBatchKernelCodegenOutput`: `outputVectorClass` maps `NullType` to
`NullVector`; `emitWrite` gets a `NullType` branch that only calls `setNull`
and never reads the source value; `emitSpecializedGetterExpr` returns `"null"`
for `NullType`, keeping the emitter's type surface in sync with the gate.
- `docs/source/user-guide/latest/scala_java_udfs.md`: `NullType` arguments
stay in the unsupported list, `NullType` return types are documented as
supported. While editing that bullet I also removed `CalendarIntervalType` from
the unsupported list: the list dates from #4267, and #4898 later added
`CalendarIntervalType` to the codegen gate and both emitters
(`CometBatchKernelCodegen.scala`, `CometBatchKernelCodegenInput.scala`,
`CometBatchKernelCodegenOutput.scala`) without touching this doc, so the entry
has been stale since then. This is a doc-only correction with no code change.
- `CometCodegenSourceSuite`: five new unit tests — `canHandle` accepts
`NullType` outputs (top-level and nested), rejects `NullType` inputs (top-level
and nested), the `NullType` output path writes `setNull` without reading a
source, nested `NullType` output casts the child vector, and the gate and
output emitters agree across the whole accepted type surface.
- SQL file tests: 10 new queries in `map/create_map.sql` (`map()`, `map('a',
NULL)`, `map(k, NULL)` over a table, nesting in `array` / `struct` / `map`,
`map_from_arrays(array(), array())`, `size` / `map_keys` / `map_values` over
them, and a `map()` column carried through `ORDER BY` so the vector survives
copy/spill paths), plus one untyped empty-constructor query in each of
`array_sort_comparator.sql`, `transform.sql`, `zip_with.sql`, `map_concat.sql`,
`map_filter.sql`, `map_zip_with.sql`, `transform_keys.sql`,
`transform_values.sql`. These run through `checkSparkAnswerAndOperator`, so a
Spark fallback would fail them.
## How are these changes tested?
Run locally with the `spark4.1_2.13` profile after rebuilding the native
library:
- `make core` — native rebuild so the JVM suites exercise the current FFI
path.
- `./mvnw test -Dtest=none
-Dsuites="org.apache.comet.CometCodegenSourceSuite,org.apache.comet.CometSqlFileTestSuite"`
— `Tests: succeeded 526, failed 0`, BUILD SUCCESS. All 9 modified SQL fixtures
passed, which is the end-to-end check that `NullVector` (including
`MapType(NullType, NullType)`) round-trips through FFI and native execution
without a fallback.
- scalastyle and spotless `check` ran as part of that build and passed.
Both `CometCodegenSourceSuite` and `CometSqlFileTestSuite` are already
registered in the CI suite matrices, so no workflow change is needed.
Assisted-by: Claude Code (claude-fable-5)
--
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]
