andygrove commented on PR #5300:
URL:
https://github.com/apache/datafusion-comet/pull/5300#issuecomment-5441662789
> **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.
Good follow-up. Skipping the `length` kernel entirely for `LargeList` rather
than paying for an `Int64` array plus a cast is the obvious win once you see
the numbers, and I like that the fast path is guarded on the total span fitting
in `i32` with a per-row fallback that keeps the same error contract.
Two things.
**The audit doc is accumulating history**
`collection_funcs.md` now has three "Performance (tuned ...)" bullets for
`spark_size`, and the first two describe implementations that no longer exist.
The contributor-guide expression audits are meant to describe the current state
of an expression, not its changelog, and a reader trying to understand what
`spark_size` does today has to work out which of the three bullets still
applies.
Could these be collapsed into one bullet that describes the current
implementation, with the PR links kept as references? Something like: List and
FixedSizeList use Arrow's `length` plus an `into_parts` null rewrite, LargeList
and Map subtract adjacent offsets directly, with the measured speedups and the
benchmark file named once.
**The `length` kernel type check is now a hard error**
```rust
if !matches!(lengths.data_type(), DataType::Int32) {
return exec_err!("unexpected type from length kernel: {:?}", ...);
}
```
Previously an `Int64` result was cast. Now anything other than `Int32` fails
the query. The comment argues `FixedSizeList` always yields `Int32` and
`LargeList` is routed elsewhere, which I believe is true for the current Arrow
version. But if a future Arrow release changes `FixedSizeList`'s output type,
this becomes a runtime failure rather than a silently correct cast, and it will
surface as a Comet bug report rather than a build break.
Is that trade deliberate? If so, saying "fail loudly rather than silently
paying for a cast we do not expect" in the comment would make it clear it is a
choice. If the goal is just to drop dead code, keeping a cast for the
unexpected case costs nothing on the hot path since it never runs.
--
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]
