andygrove commented on PR #5233: URL: https://github.com/apache/datafusion-comet/pull/5233#issuecomment-5192606658
Thanks for the revisions. Every item from the last round is addressed, and the benchmark story is much better now that the no-null shape is in. I ran the checks locally on `e28ea8c`: - `cargo test -p datafusion-comet-spark-expr size`, 11 passed - `cargo clippy -p datafusion-comet-spark-expr --all-targets -- -D warnings`, clean - `cargo bench --bench array_size` The benchmark numbers reproduce on my machine, against the `main` numbers I measured last time: | shape | main | this PR | change | | --- | --- | --- | --- | | list of short arrays (10% null) | 9.30 µs | 1.63 µs | 5.7x | | list of long arrays (10% null) | 9.93 µs | 1.61 µs | 6.2x | | list, no nulls | 6.67 µs | 0.56 µs | 12x | | LargeList (10% null) | 9.70 µs | 7.54 µs | 1.3x | I also went back through the two things that worried me about moving to buffer-level ops, and both hold. I read the `length` kernel in `arrow-string` 58.4.0 to confirm it covers all three types the dispatch now routes to it, and that `FixedSizeList` carries the input null buffer through. The `set_indices` patch is offset-correct because `Not` on a `&BooleanBuffer` rebuilds at offset 0 with the sliced length, which is exactly what `test_spark_size_sliced_list_array` pins. And the `safe: false` cast on the `LargeList` path visits null slots too, but a null list row's offsets still give a small in-range length, so it cannot raise a spurious overflow. Two things I would like to see before merge. **A tracking issue for the `LargeList` path.** Thanks for opening #5266 for Map. `LargeList` still needs one. It is at 1.3x while `List` gets 12x, and the reason is visible in the code: the kernel allocates an Int64 array, `cast_with_options` allocates a second, and `to_vec` allocates a third. Building Int32 straight from the offsets would close that. I do not want this to hold the PR up, but without an issue it will not get picked up. **The comment at `size.rs:164`.** It says `into_parts` moves the values buffer without copying and that the work is `O(null_count)` rather than `O(n)`. Line 171 is `values.to_vec()`, which is a full `O(n)` copy, and `int_lengths.clone()` bumps the buffer refcount while `lengths` is still alive so there is no way to reclaim it as written. The `set_indices` loop itself is genuinely `O(null_count)`, but the function is not. Could you reword it? `optimizing_expressions.md` now points contributors here as the reference for this technique, so I would rather the comment not promise more than the code delivers. -- 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]
