andygrove commented on PR #4672: URL: https://github.com/apache/datafusion-comet/pull/4672#issuecomment-5441754295
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled. Replacing the per-element `is_null_in_bitset` probe with a bulk byte-level flip plus a `NullBuffer` is the right idea, and keeping the unaligned pointer path as a fallback rather than assuming alignment is important given the `unsafe_object.rs` note. Four things. **Does `append_array` care about the builder's timezone?** The timezone is now threaded through `append_timestamps_to_builder` and applied with `with_timezone_opt` before `builder.append_array(&arr)`. That suggests `append_array` compares data types and would reject a mismatch. Is that right? If so, is there a test with a non-UTC session timezone, so we know the threading actually matches what the builder was constructed with? If `append_array` does not check, then `with_timezone_opt` is doing nothing and the extra parameter can go. **The unaligned path is now untested** There are two implementations per type that must produce identical results, and the new tests construct buffers through `make_i32_array`, which I would expect to be aligned. So the aligned bulk path is covered and the per-element fallback is not, even though it is the one that runs when the JVM hands over a misaligned buffer. Could one test force the misaligned case, for example by allocating with one extra byte of padding and offsetting the pointer? Even one test per type asserting the two paths agree on the same logical data would be worth having, since a divergence between them would be a wrong answer rather than a crash. **Trailing bits in the flipped mask** `flipped` inverts whole bytes, so for a `num_elements` that is not a multiple of 8 the padding bits in the last byte become 1. `BooleanBuffer::new(buffer, 0, num_elements)` bounds the logical length so those bits should be ignored, and `null_count` should be computed only over the logical range. I convinced myself this is fine, but it is the kind of thing worth a test with a deliberately awkward length such as 3, 9, or 65, mixing nulls into the final partial byte. **Benchmark numbers should be in the description** The description says the results are "copied from the comment below", but the body itself has no table. Comments get buried and do not survive into the commit history. Could you inline the before and after medians for each type and shape, and state explicitly that the `no_nulls` shapes are unaffected? That last point matters because it tells a reader the win is scoped to nullable arrays. -- 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]
