andygrove commented on PR #5300:
URL:
https://github.com/apache/datafusion-comet/pull/5300#issuecomment-5256103735
_This review was drafted with LLM assistance (Claude Code) and edited before
posting._
Nice follow-up to #5233. I verified the offset-subtraction approach against
Arrow 58.4.0's own `length_impl` in `arrow-string/src/length.rs`, which uses
`offsets.windows(2).map(|w| w[1].sub_wrapping(w[0])).collect()`. Producing
`i32` directly is the same shape, just skipping the Int64 return + cast. `cargo
test -p datafusion-comet-spark-expr --lib -- spark_size` passes 14/14 on the
branch.
A few things:
**Missing docs change.** The description mentions adding a row to
`docs/source/contributor-guide/expression-audits/collection_funcs.md`, but only
the two Rust files are in the diff. Did that get dropped in a rebase?
`optimizing_expressions.md` asks for a dated `Performance (tuned ...)` line
naming the technique, speedup, PR, and benchmark file. Worth noting that #5233
did not add one either, so if you're in there anyway it would be good to record
both passes under `## size` so the history stays complete.
**Dead `Int64` cast arm in `spark_size_list_like`.** Now that `LargeList`
has its own path, this helper only ever sees `List` and `FixedSizeList`.
Arrow's `length()` dispatches `List` to `length_impl::<Int32Type>` and
`FixedSizeList` builds an `Int32Array` directly, so `lengths.data_type()` can
only be `Int32` here. That makes the `DataType::Int64 =>
cast_with_options(...)` arm unreachable, along with the `cast_with_options` and
`CastOptions` imports (not used anywhere else in the file). Could we drop the
arm and the imports? You already removed the LargeList sentence from the doc
comment on this function, and removing the cast machinery too would make the
point of the PR visible in the code. The `other => exec_err!` arm still catches
anything unexpected if a new list type gets routed here later.
**Duplicate null-count guard in the new function.**
```rust
if list.null_count() == 0 {
return Ok(Arc::new(Int32Array::from(values)));
}
let nulls = list.nulls().unwrap();
Ok(Arc::new(ints_with_nulls_as_neg_one(values, Some(nulls))))
```
`ints_with_nulls_as_neg_one` already skips the rewrite when `null_count()`
is zero and when `nulls` is `None`, and both branches end in
`Int32Array::from(values)`, so this collapses to
`Ok(Arc::new(ints_with_nulls_as_neg_one(values, list.nulls())))`. Since the
reason for extracting that helper was so the `List` and `LargeList` paths
cannot drift, keeping a second copy of the guard here works against that.
**Test coverage gaps on the branches this PR adds.**
- Both LargeList array tests use inputs with a null row, so the
`null_count() == 0` early return never runs under test. That is the shape you
describe as the production path and benchmark at 693 ns, so it seems worth a
`test_spark_size_large_list_array_no_nulls` mirroring the existing
`test_spark_size_array_no_nulls`, asserting `null_count() == 0` on the output.
- For the checked fallback, `test_spark_size_large_list_length_overflow`
pins the error and `..._checked_null_row_skips_overflow` pins the null skip,
but I do not see a case where a non-null row's length actually fits while the
overall span overflows. That is the reason the fallback loops per row instead
of just erroring outright when `range > i32::MAX`, so it seems worth pinning.
Something like `OffsetBuffer::new(vec![0i64, i32::MAX as i64, i32::MAX as i64 +
10].into())` should give `[i32::MAX, 10]` and would catch a future change that
turns the span check into a hard error.
The overflow error text change is fine given `CometSize.convert` wraps the
call in `CASE WHEN isnotnull(child)` and Spark caps arrays at `Int.MaxValue`.
No compatibility concern there, and support levels do not change so
`getIncompatibleReasons()` / `getUnsupportedReasons()` stay accurate.
--
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]
