comphead commented on code in PR #6132:
URL: https://github.com/apache/datafusion-comet/pull/6132#discussion_r4088645136
##########
native/core/src/execution/operators/explode.rs:
##########
@@ -943,13 +941,22 @@ fn find_longest_length(list_arrays: &[ArrayRef], options:
&UnnestOptions) -> Res
} else {
Scalar::new(Int64Array::from_value(0, 1))
};
+ let expand_empty = options.expand_empty_as_null();
+ let zero = Scalar::new(Int64Array::from_value(0, 1));
+ let one = Scalar::new(Int64Array::from_value(1, 1));
let list_lengths: Vec<ArrayRef> = list_arrays
.iter()
.map(|list_array| {
let mut length_array = length(list_array)?;
// Make sure length arrays have the same type. Int64 is the most
general one.
length_array = cast(&length_array, &DataType::Int64)?;
length_array = zip(&is_not_null(&length_array)?, &length_array,
&null_length)?;
+ if expand_empty {
+ // Bump empty lists to length 1 so they produce a single
NULL-padded output row.
+ // Runs after the NULL substitution above, which has already
set NULL rows to 1,
+ // so they are not matched here.
+ length_array = zip(&eq(&length_array, &zero)?, &one,
&length_array)?;
Review Comment:
Both applied, thank you.
**The hoist.** The identity holds, so I moved it: `max(max(a, 1), max(b,
1))` and `max(max(a, b), 1)` are both `max(a, b, 1)`. In the code the old form
computed `max_i max(s_i, 1)` and the new one computes `max(max_i s_i, 1)`.
It turned out to be a simplification beyond the saved work. The `map`
closure is now byte-identical to upstream again, so the divergence is a single
labeled step after the fold rather than a condition threaded through the loop,
and the divergence list in the module docs got more precise.
Your comment also exposed a test gap. Every existing test passed exactly one
array to `find_longest_length`, so nothing covered the fold this moves across.
Added `longest_length_combines_arrays_before_the_empty_bump`, with a fixture
chosen to discriminate: a row empty in one array but 2 long in the other (the
bump must not inflate it past the other array), a row empty in both (the only
row where `PreserveAndExpandEmpty` differs from `Preserve`), and a row empty in
one and NULL in the other. Worth noting that the per-array form also passes all
of these, which is independent confirmation that the two are equivalent rather
than just my algebra.
**The positional benchmark.** Added, and you were right that nothing
measured it: every arm built a single `ListUnnest`, and the outer arms held
only NULL rows so the empty-row substitution was unmeasured too.
`posexplode_fan_out` now unnests a parallel positions column alongside the
array, over a new `RowMix` shape that mixes NULL and empty rows, at fan-out 2
and 10.
I could not get a number worth reporting from it. Criterion comparing an
identical binary against itself on my machine gives:
```
posexplode_fan_out/dense/2 +20.6% p=0.00 "Performance has
regressed"
posexplode_fan_out/dense/10 -16.4% p=0.00 "Performance has
improved"
posexplode_fan_out/nulls_and_empties/2 -10.2% p=0.00 "Performance has
improved"
posexplode_fan_out/nulls_and_empties/10 -15.8% p=0.00 "Performance has
improved"
```
A ±20% noise floor with confident p-values, from no code change. Note that
`dense/*` runs under `NullHandling::Drop` and `explode_outer_with_nulls` has
one list column, so neither can be affected by this transform at all, which is
how I spotted that the readings were noise rather than signal.
Your instinct to distrust the debug-dependency probe was right, and it
applies to my own earlier numbers too: the percentages I had quoted for
`explode_outer_with_nulls` in the description came from the same environment,
so I have removed them rather than leave an unsupportable claim in place. The
benchmark arms are committed, so the measurement is reproducible by anyone with
a quiet machine.
On the underlying question of whether the positional outer path got slower:
it now does one `eq`/`zip` pair per batch that the old code did not, and the
old code additionally ran a whole `ListEmptyToNullExpr` projection per batch
that this PR deletes. The operator-only benchmark cannot see that projection,
so it understates the change in the planned query in the PR's favour. I did not
want to claim a win on that basis without measuring it.
--
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]
