andygrove opened a new pull request, #5820:
URL: https://github.com/apache/datafusion-comet/pull/5820
Backport of #5737 to `branch-1.0`.
Cherry-picked from `cbe3b39035a5918fc65496ebfe08bcf8e7adfce2`. The planner
change is
byte-identical to the original PR; only the new Rust test needed adaptation,
described under
"What changes are included" below.
Note that this is a performance enhancement rather than a bug fix, unlike
the other backports on
this branch so far. Including it in 1.0.1 is a release-management call, not
something the change
itself forces.
## Which issue does this PR close?
Closes #5676 on `branch-1.0`.
## Rationale for this change
Plain `posexplode` places its array expression in both the positions
expression and the values
projection, evaluating it twice per batch. `posexplode_outer` already
materializes its wrapped
array once. For a computed array, the duplicate evaluation can repeat native
work or a JVM
codegen-dispatch call.
## What changes are included in this PR?
The planner change is the original one: apply the outer wrapper first, then
materialize the array
when positions are requested and the expression is not already a `Column`,
so both positions and
values reference the resulting column. Plain array-column inputs keep their
existing single
projection, and non-positional explode variants are unchanged. The
intermediate column prefix is
renamed to `__comet_explode_`.
Two adaptations were needed for `branch-1.0`, both confined to the new Rust
test:
- Added the `AtomicUsize`/`Ordering`, `ArrayRef` and `MemorySourceConfig`
imports the test needs.
On `main` those were already in the test module, brought in by tests added
after `branch-1.0`
was cut.
- Dropped `element_field_id: None` from the test's `ListInfo` literal. That
proto field does not
exist on `branch-1.0`.
`branch-1.0` pins the same DataFusion version (54.1.0) as `main`, so no API
adaptation was needed.
## How are these changes tested?
Same tests as the original PR, verified locally on `branch-1.0`:
- `cargo test -p datafusion-comet` passes 156 tests, including the new
`explode_evaluates_array_once_per_batch` planner regression.
- The regression was confirmed to fail on `branch-1.0` with the planner
change reverted and the
test kept, reproducing the count from the original PR: plain positional
explode over a computed
array evaluates the function 4 times for two batches instead of 2. So the
duplicate evaluation
is present on `branch-1.0` and this backport is what removes it.
- `CometGenerateExecSuite` passes all 38 tests on Spark 4.1.3 / Java 17,
including the new
`posexplode with a computed array from Parquet`.
- `CometSqlFileTestSuite` `expressions/array/posexplode.sql` passes.
- `cargo clippy --all-targets --workspace -- -D warnings` and `cargo fmt
--all -- --check` are
clean, and Spotless and Scalastyle pass as part of the Maven run.
- `test-compile -Pspark-3.4 -Pscala-2.12` succeeds, so the new Scala test
cross-compiles on the
oldest supported Spark/Scala combination.
## Are there any user-facing changes?
No. Results are unchanged; a computed array passed to `posexplode` is now
evaluated once per batch
instead of twice.
--
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]
