comphead opened a new pull request, #6132:
URL: https://github.com/apache/datafusion-comet/pull/6132
## Which issue does this PR close?
Closes #5210.
## Rationale for this change
DataFusion 55.1.0, which Comet already pins, carries `unnest_outer`
(apache/datafusion#22100) as `NullHandling::PreserveAndExpandEmpty`: an
empty input list
produces one NULL output row, exactly like a NULL list does.
That is Spark's `explode_outer` semantics, so Comet no longer needs its own
bridge. Before
this PR the planner wrapped the array child in `ListEmptyToNullExpr`, a
Comet-only
`PhysicalExpr` that rewrote every empty row to NULL so that `preserve_nulls`
would pad it.
The planner can now ask DataFusion for the semantics directly.
No user-visible behavior changes. `explode_outer` and `posexplode_outer`
already ran
natively.
## What changes are included in this PR?
Planner (`native/core/src/execution/planner.rs`):
- Drops the `ListEmptyToNullExpr` wrapper.
- Selects `NullHandling::PreserveAndExpandEmpty` when `explode.outer`, and
`NullHandling::Drop` otherwise. The non-outer arm is unchanged, since
`with_preserve_nulls(false)` already mapped to `Drop`.
Operator (`native/core/src/execution/operators/explode.rs`):
- Ports upstream's empty-row bump into `find_longest_length`, which serves
`posexplode`.
- Makes the matching change in `list_output_lens`, Comet's fused
single-`List` fast path,
which serves `explode`. The two must agree element for element, because
`predict_output_lens` uses the result both to place chunk boundaries and as
`precomputed_lengths` for `build_batch`.
- No change to `unnest_list_array` or `create_take_indices`. Upstream
carries the whole
feature in the length array, and the existing pad loop already emits the
NULL. The
contiguous-run fast path self-disqualifies for a padded row, because an
empty row adds 1
to `capacity` and 0 to the offset span.
Deletes `native/core/src/execution/expressions/list_empty_to_null.rs` (311
lines).
The module docs on the fork were rewritten. They said to delete the fork
once Comet moved
to a release carrying apache/datafusion#24384. Comet is now on such a
release, but the fork
still carries two paths that were never upstreamed: `list_output_lens` and
the
contiguous-run slice in `unnest_list_array`. The docs now say that, so the
next person does
not delete the file and silently regress `explode`.
## How are these changes tested?
Existing coverage is the real gate, and it is unchanged: `explode.sql` and
`posexplode.sql`
cover `explode` / `explode_outer` / `posexplode` / `posexplode_outer` across
every primitive
element type, nested arrays and structs, NULL and empty arrays, `LATERAL
VIEW [OUTER]`, and
a map `expect_fallback`. `CometGenerateExecSuite` covers batch boundaries,
sliced inputs
from limit/offset, and single rows that exceed the batch size.
The native planner test `explode_evaluates_array_once_per_batch` already
asserts the full
Spark contract over `[[10, null], [], null, [20]]` for all eight
combinations of
`(outer, position, computed)`, including `pos = [0, 1, null, null, 0]`.
Added in this PR:
- `output_lens_substitute_per_null_handling` pins the absolute per-row
lengths for all three
`NullHandling` modes and checks the fused and general kernels against
them, so a mistake
made in both at once cannot pass.
- `output_lens_handle_a_sliced_input` gains a sliced fixture that also has a
validity buffer.
- `contiguous_unnest_covers_empty_rows_and_dropped_nulls` gains the outer
case, asserting
that the fast path is rejected once rows are padded.
- The chunking-equivalence tests now include empty rows and run under the
two modes the
planner can actually produce.
Both length functions were mutation-tested: disabling the bump in either one
fails the new
tests.
Spark behavior was verified against the Spark source rather than assumed.
`ExplodeBase.eval`
returns an empty collection for both a NULL and an empty array, and
`GenerateExec`
substitutes an all-NULL generator row, so the two cases are
indistinguishable and `pos` is a
true NULL rather than 0 or -1. The codegen path reaches the same result
through an
`index == -1` sentinel. This is unchanged across 3.4.3, 3.5.8, 4.0.x, and
4.1.1.
Performance: `native/core/benches/explode.rs`, comparing this branch against
its merge base.
No regression on any arm. The `explode_outer_with_nulls/bigint` arm, which
exercises the
per-row change most directly, came in at -1.7% to -3.1% over three runs.
Run-to-run spread
is wide enough that this is best read as no regression rather than as a win.
The benchmark
constructs `ExplodeExec` directly, so it does not capture the
`ProjectionExec` and expression
evaluation that this PR removes from the planned query.
One plan-shape change: `posexplode_outer` over a plain column no longer
needs the
pre-projection, because the wrapper was what made the child a non-`Column`.
That is one
fewer `ProjectionExec`. No plan-stability golden file covers `explode`.
--
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]
