andygrove commented on PR #5215: URL: https://github.com/apache/datafusion-comet/pull/5215#issuecomment-5441597288
> **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. This is good hardening. Replacing the hard `downcast_ref::<StringArray>().expect(...)` with a match that returns `internal_err!`, and switching the dictionary path from `DictionaryArray<Int32Type>` to `as_any_dictionary()` so any key type works, both remove real panic paths. Keeping the plan-time compiled `Regex` rather than moving to Arrow's kernel is the right call and the rustdoc explaining why is worth having. Two things. **`collect()` now always produces a null buffer** The old code had two branches, and the non-nullable one used `append_value` throughout, so `BooleanBuilder::finish` produced an array with no null buffer at all. The new `inputs.iter().map(|v| v.map(...)).collect()` goes through `FromIterator<Option<bool>>`, which as far as I can tell allocates a validity buffer regardless. For an all-valid input that is an extra allocation of `len / 8` bytes per batch, and more importantly some downstream Arrow kernels take a faster path when `nulls()` is `None` rather than when `null_count() == 0`. Since `RLike` output feeds directly into filters, that could matter. Could `test_rlike_string_array_no_nulls` assert `result.nulls().is_none()` rather than just checking the values? If it turns out a buffer is allocated, keeping the two-branch form for the non-nullable case would preserve the old behavior while still getting the layout generality from `StringArrayType`. **Reachability** The description is honest that `LargeUtf8` and `Utf8View` may not be reachable end to end today. That is fine for hardening, but it does mean the new code paths are exercised only by the Rust unit tests. Is it worth adding a `#[test]` that runs a `Utf8View` input through the dictionary path as well, since that is the combination with the most moving parts (any key type, view values, `take` over the result)? The current dictionary tests use `Int32Type` and `Int8Type` keys but I did not see one with view values. -- 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]
