peterxcli commented on PR #5174:
URL:
https://github.com/apache/datafusion-comet/pull/5174#issuecomment-5443540388
@andygrove thanks for the pass — evaluated all three.
**1. Description vs. code on null defaults** — you're right, the description
was stale. Explicit null defaults were moved onto the `take` fast path in the
second revision (per the earlier review's point 1), and the description still
described the first revision. I've updated it to match the code.
**2. `check_bounds`** — arrow's `TakeOptions` derives `Default`, so
`check_bounds` is already `false` when passing `None` (verified in arrow-select
58.4.0, `take.rs`); there's no bounds-checking work to remove. I added a
comment at the `take` call noting that every gathered index is pre-validated by
`resolve_row` and that `take` masks null index slots (`take_native` returns
`T::default()` for an out-of-bounds placeholder under a null slot rather than
panicking), so the unchecked default is safe.
**3. Coverage** — the checks you describe exist, mostly from the previous
round:
- `test_list_extract_default_value` and `test_list_extract_null_index` run
the same data through all three configurations (no default, explicit null
default, non-null default) and assert the outputs, so the two implementations
are pinned against each other at the unit level.
- `test_list_extract_without_default_ansi_errors` asserts
`InvalidElementAtIndex` (one-based) and `InvalidArrayIndex` (zero-based)
specifically on the no-default/`take` path; `element_at_ansi.sql` and
`get_array_item_ansi.sql` cover both end to end.
- At the SQL level a non-null default can't be expressed through `arr[i]` or
`element_at` — the only Spark path that sets `defaultValueOutOfBound` is
`split_part`, and `split_part.sql` already exercises out-of-bounds part numbers
(e.g. `split_part('a.b', '.', 4)`).
--
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]
