andygrove commented on PR #5192:
URL:
https://github.com/apache/datafusion-comet/pull/5192#issuecomment-5168698227
Thanks for the updates. Everything from my last round is addressed, and
addressed well.
- The `unsafe` `NullBuffer::new_unchecked` is gone in favor of the safe
`NullBuffer::new(combined)`.
- `list_empty_to_null.rs` now has a seven-test module covering both fast
paths, mixed empty/null/non-empty rows, empties on top of a pre-existing
bitmap, a zero-row batch, sliced input with a non-zero offset, and
`return_field` nullability.
- The pre-projection column is named `__comet_explode_outer_<name>`, so the
duplicate field name is gone.
- The pre-projection is gated on `explode.position`, so plain
`explode_outer` no longer pays for a `ProjectionExec` it cannot share.
- Both query shapes I asked about are in `explode.sql` and mirrored in
`posexplode.sql`, plus the two new batch-boundary tests with `COMET_BATCH_SIZE
= 4`. Nice addition.
- The `native_shuffle.md` churn is reverted and the `expressions.md` wording
now reads "Enabled by default".
Four things left.
### CI: scalafix
The four `Lint Java` jobs are failing in `Run scalafix check`, and it is
this PR. Removing the `op.outer` branch from `CometExplodeExec.getSupportLevel`
left `Incompatible` imported but unused in
`spark/src/main/scala/org/apache/spark/sql/comet/operators.scala` at line 61.
The only other match in the file is a string literal. `.scalafix.conf` enables
`RemoveUnused`, so dropping `Incompatible` from that import list should clear
all four jobs.
The `Spark SQL Tests (Spark 3.5) / spark-sql-sql_core-3` failure is not
yours. It failed in `Setup Spark` after 1m36s while the other six 3.5 shards
ran the same step and passed. That one just needs a re-run.
### #5224 is now reachable by default
To answer your question above: yes, I would fix both in this PR rather than
split it. `native/core/src/execution/expressions/list_positions.rs` is
unchanged here, so it still builds a fresh values array numbered from zero
while reusing the input's original offset buffer. A `ListArray` with a non-zero
offset base panics inside `ListArray::new`. Today `posexplode_outer` falls back
so users are safe. Once it runs natively it hits the panic. The fix is small
and it covers plain `posexplode` at the same time:
```rust
let offsets = list.offsets();
let base = offsets.first().copied().unwrap_or(0);
let total_len = (*offsets.last().unwrap() - base) as usize;
// ... build values as today ...
let rebased = OffsetBuffer::new(offsets.iter().map(|o| o -
base).collect::<Vec<_>>().into());
let result = ListArray::new(
element_field,
rebased,
Arc::new(Int32Array::from(values)),
list.nulls().cloned(),
);
```
Pair it with the `posexplode_outer over limit with offset` test from my
earlier comment. If you would rather keep the fix separate, please add that
test as `ignore` referencing #5224 so it is tracked rather than silent.
### The TODO placeholder is still literal
`planner.rs` still says `(TODO: link the Comet tracking issue here)`. I
searched and no such issue exists yet. Could you file one for removing
`ListEmptyToNullExpr` and the pre-projection once
[datafusion#19053](https://github.com/apache/datafusion/issues/19053) lands,
then drop the link in place of the placeholder? A `TODO: link` shipped in a
comment leaves nothing pointing at the workaround.
### Benchmark numbers
Still missing. This change flips `explode_outer` and `posexplode_outer` from
falling back to running natively by default, and there is no `GenerateExec`
benchmark in the repo. Even ad-hoc timings in the description for an array
column with a mix of empty and non-empty rows would confirm the native path is
a win and quantify what the extra pass costs.
--
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]
