andygrove commented on PR #4672:
URL:
https://github.com/apache/datafusion-comet/pull/4672#issuecomment-4743026147
Thanks for picking this up. The shape of the change is exactly what #4626
was asking for: keep the runtime alignment check as load-bearing, and replace
the per-element null branch with `PrimitiveBuilder::append_values(values,
&is_valid)`. The benchmark numbers look great.
A few things to look at, the first one is a correctness concern.
1. **Boolean nullable path may have UB.** In `append_booleans_to_builder`,
the new nullable path casts `*const u8` to `*const bool` and iterates:
```rust
let slice = unsafe { std::slice::from_raw_parts(self.element_offset as
*const bool, num_elements) };
for (idx, &value) in slice.iter().enumerate() { ... }
```
Rust requires every `bool` value to be exactly `0` or `1`. For null
slots, the underlying byte is JVM-uninitialized memory, so the `let &value`
materialization in the iterator pulls out a `bool` from arbitrary bytes which
is UB even when the value is then ignored. The non-nullable branch a few lines
down correctly uses `*const u8` plus `value != 0`. Could you mirror that
pattern in the nullable branch? `cargo +nightly miri test` would give a
definitive answer, and it might be related to the UB you mentioned hitting
earlier.
2. **Boolean nullable path still branches per-element.** Once the cast is
fixed, the loop still does a per-element null check, which is the shape #4626
specifically called out as the bigger win for booleans.
`BooleanBuilder::append_values(values: &[bool], is_valid: Option<&[bool]>)`
would absorb the validity in bulk and remove the branch. Worth taking here if
straightforward.
3. **`Vec<bool>` allocation per call.** The simpler `Vec<bool>` form is fine
for landing, but #4626 suggested feeding a `BooleanBuffer` directly to the
builder to skip the bool materialization. If you'd rather defer that, a
follow-up issue with the link to the previous discussion would be helpful. At
minimum a `Vec::with_capacity(num_elements)` instead of `collect` would avoid
one growth.
4. **Stale assert message in `append_dates_to_builder`.** The new
`debug_assert!` message reads `"append_timestamps: element_offset is null"`.
Copy-paste from the timestamp path, should be `append_dates`.
5. **Comment phrasing on the alignment fallback.** Could you reword the `//
Note: alignment is not guaranteed - that is why do this` comments to point at
the existing explanation at `list.rs:105` and say why (nested-array
variable-length region)? The prior PR was bounced because the description left
the runtime check looking redundant, and a clear comment here is the easiest
way to keep that from happening again.
6. **PR description.** The benchmark numbers in your comment look like the
right shape. Could you pull them into the PR description body and mention they
came from `cargo bench --bench array_element_append`?
I'll approve the CI workflows so the matrix actually exercises the change.
Out of curiosity, what was the UB you hit earlier? Worth knowing whether item 1
above was the cause or there's a separate hazard.
---
*Disclosure: this review was assisted by an AI tool (Anthropic Claude via
Claude Code). I read the diff, the linked issue, and the prior closed PR's
review feedback before forming the comments above, and I take responsibility
for them.*
--
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]
