andygrove commented on PR #4932:
URL:
https://github.com/apache/datafusion-comet/pull/4932#issuecomment-5441568309
> **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.
The allocation reduction is real and the flamegraphs make the case clearly.
Threading a scratch buffer owned by the accumulator, swapping into it, and
clearing is the right shape for this.
Three things.
**The `self.count == 0` branch hand-copies every field**
```rust
if self.count == 0 {
self.compress_threshold = other.compress_threshold;
self.relative_error = other.relative_error;
self.sampled.clear();
self.sampled.extend_from_slice(&other.sampled);
self.count = other.count;
self.compressed = other.compressed;
self.head_sampled.clear();
return;
}
```
That covers all six fields today, but it is exactly the kind of code that
silently drops a field when someone adds a seventh. `QuantileSummaries` already
derives `Clone`, and `Clone::clone_from` on a derived impl does field-wise
`clone_from`, which for `Vec` reuses the existing allocation. So
`self.clone_from(other); return;` should give you the same allocation reuse in
one line and stay correct when the struct grows. Is there a reason that does
not work here?
**`pub` to `pub(crate)` on the public API**
`insert`, `compress`, and `merge` all become `pub(crate)`, while
`QuantileSummaries` itself stays `pub`. That leaves a public type with no
public way to use it. If `datafusion-comet-spark-expr` is consumed outside this
repo, this is a breaking change; if it is not, the type should probably become
`pub(crate)` too. Either way, having the type public and every method
crate-private is a state worth resolving rather than leaving.
**No latency measurement**
The title says `perf` and the evidence is allocated bytes. Fewer allocations
usually means faster, but not always, and the swap-and-clear pattern adds some
bookkeeping on paths that previously just moved a `Vec`. Could you add a
Criterion or wall-clock number for `merge_batch` and for a full
`approx_percentile` over a realistic batch? Even a rough before and after would
turn "allocates less" into "is faster", which is what the PR claims.
**One smaller question**
`QuantileSummariesScratch.sampled` grows to the high-water mark across every
summary it serves and never shrinks. For a groups accumulator with many groups
that is one buffer sized to the largest group, which seems fine. Is `heap_size`
counting it in a way that avoids double counting against the summaries
themselves? The memory accounting change is mentioned in the description but it
would help to say in a comment which allocation each `heap_size` covers.
--
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]
