andygrove commented on PR #5174:
URL:
https://github.com/apache/datafusion-comet/pull/5174#issuecomment-5441553369
> **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.
Nice result, and I appreciate that the benchmark table shows the
non-null-default path too rather than only the case that improves. Factoring
the per-row decision into `resolve_row` so both implementations share the index
arithmetic and the ANSI error handling is a good way to keep them from drifting.
Three things.
**The description contradicts the code on explicit null defaults**
The description says the original `MutableArrayData` path is retained
"whenever an explicit default is supplied, including an explicit null default".
The code does the opposite:
```rust
let Some(default_value) = default_value.filter(|default| !default.is_null())
else {
return list_extract_without_default(...)
};
```
An explicit null default falls through to the `take` path. That is the right
behavior, since a null default and no default produce the same output, and it
means the fast path applies more often. But the description should say so,
because as written a reader would go looking for the branch that does the
opposite.
**Is `check_bounds` worth disabling?**
`take(values.as_ref(), &indices, None)` uses default `TakeOptions`. Every
index handed to `take` has already been validated by `adjust_index` against the
row's list length, so the bounds check is redundant work proportional to the
batch size. If Arrow's default is bounds-checking, passing `TakeOptions {
check_bounds: false }` should be free speed. If the default is already
unchecked, a one-line comment saying the indices are pre-validated would be
worth having anyway.
**Coverage of the new split**
`get_array_item.sql` gains 15 lines. Since the change introduces two
implementations that must agree, it would be good to know the SQL fixture
exercises both. Specifically: a query with an explicit non-null default and
out-of-bounds ordinals, and the same query without a default, against the same
data. If those two are not both there, a future change to `resolve_row` could
break one path silently.
Related, does anything cover `element_at` (one-based) versus `arr[i]`
(zero-based) through the new `take` path, in ANSI mode, where the two produce
different error types? `out_of_bounds_error` is only reachable from
`resolve_row` now, so one test per branch would pin it.
--
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]
