andygrove commented on PR #4727:
URL:
https://github.com/apache/datafusion-comet/pull/4727#issuecomment-5441761922
> **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.
Replacing a conservative "force the whole chain back to Spark" guard with
code that actually understands the buffer is the right direction, and deleting
`hasNativeArrayBufferAgg` plus its tagging block is a satisfying amount of
removal. The `accumulator_expr_fields` override, so the collect accumulator
sees the original scalar input type rather than the state type, is a subtle
point that is well explained in the comment.
Four things.
**`try_new(...).unwrap_or(Self::PassThrough)` swallows real failures**
```rust
SparkCollectStateDecoder::try_new(inner_expr, state_fields)
.map(Self::SparkCollect)
.unwrap_or(Self::PassThrough)
```
This conflates two very different outcomes: "this is not a collect
aggregate, pass through" and "this is a collect aggregate but something about
it was unexpected". In the second case the accumulator then receives Spark
`BinaryType` buffers it will interpret as list state, which is exactly the
class of failure this PR exists to fix, except now it happens silently.
Could `try_new` return `Result<Option<...>>`, with `Ok(None)` for "not
applicable" and `Err` for "applicable but malformed"? Then the `Err` case
surfaces rather than degrading.
**Parsing Spark's `UnsafeRow` layout from native code**
`spark_aggregate_state.rs` reads a serialized single-field `UnsafeRow` and
pulls the `UnsafeArrayData` out of field 0. That is a hard dependency on
Spark's internal binary layout, decoded through `SparkUnsafeArray`, which does
raw pointer reads.
What happens on a buffer that is not the expected shape: truncated, a
different number of fields, or a null-field bitmap that says field 0 is null?
Does the code validate the length and field count before indexing, and does a
mismatch produce an error or an out-of-bounds read? This is the one place in
the PR where getting it wrong is a memory-safety problem rather than a wrong
answer, so it deserves an explicit answer in the description and a test with a
deliberately malformed buffer.
Also: which Spark versions is the layout verified against? `UnsafeRow` is
stable in practice, but the audit-style comment should say 3.4 through 4.2
explicitly if that is what was checked.
**This collides with #5420 and #5421**
Those PRs are rewriting `missingCometProducer`, the `COMET_UNSAFE_PARTIAL`
tagging pass, and `findPartialAggInPlan` in `CometExecRule`, and adding a new
post-conversion revert pass over the same predicates. This PR deletes one of
the tagging blocks and reshapes `missingCometProducer` into `sparkFinalMode`
plus `sparkPartialMergeMode`.
Whichever lands first will silently change the meaning of the other's code.
Worth coordinating with @sunchao on ordering, and worth stating in the
description which of the three is expected to go first.
**The `dev/diffs` filenames are stale relative to another PR**
This PR edits `3.5.9.diff`, `4.0.4.diff`, and `4.1.3.diff`; #4653 edits
`3.5.8.diff`, `4.0.2.diff`, and `4.1.2.diff`. One of the two is behind main.
Worth rebasing before merge so the diffs land against the current set of
supported patch versions.
--
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]
