peterxcli commented on PR #4932: URL: https://github.com/apache/datafusion-comet/pull/4932#issuecomment-5447380521
Thanks for the review @andygrove! Addressed all four points in da4edd3: **`clone_from`** — good catch on the field-drift risk. One correction though: a derived `Clone` doesn't generate a field-wise `clone_from`; it keeps the trait's default `*self = source.clone()`, which reallocates both `Vec`s and would defeat the buffer reuse. I instead wrote a manual `Clone` impl whose `clone_from` destructures the source struct (so a new field is a compile error, not a silent drop) and uses `Vec::clone_from` to reuse the destination allocations. The `count == 0` branch is now just `self.clone_from(other)`. **Visibility** — `mod quantile_summaries` is private in `agg_funcs/mod.rs` (on `main` too), and only `ApproxPercentile` is re-exported, so `QuantileSummaries` was never part of the crate's public API and no breaking change is possible here. I've normalized the markers back to uniform `pub` (scoped by the private module), matching `main`'s style, so the mixed `pub`/`pub(crate)` state is gone. **Latency** — added `benches/approx_percentile.rs` (Criterion), running a real `AggregateExec` plan in Partial and Partial→Final modes (Final exercises `merge_batch` digest deserialization + merge), scalar and grouped high-cardinality (262,144 rows / 32,768 groups). Results vs `main` on my machine (Apple Silicon, medians of 10 samples): | Benchmark | main | this PR | | |---|---|---|---| | scalar_partial | 4.55 ms | 4.51 ms | parity | | scalar_partial_final | 4.52 ms | 4.51 ms | parity | | grouped_high_card_partial | 25.84 ms | 8.70 ms | **2.97× faster** | | grouped_high_card_partial_final | 34.70 ms | 12.93 ms | **2.68× faster** | Consistent with the JVM `CometAggregateExpressionBenchmark` numbers I posted earlier (558 ms → 236 ms), and no scalar regression from the swap-and-clear bookkeeping. **`heap_size` accounting** — no double counting: buffers only move between a summary's `sampled` and the shared scratch via `mem::swap`, so each allocation has exactly one owner at any time, and `size()` sums each side once. Added comments on both `heap_size` methods and both accumulators' `size()` impls spelling out which allocation each term covers. The scratch does hold the high-water mark of the largest single flush/merge, but that's one buffer per accumulator (not per group) and it's reported through `size()` so the memory manager sees it. _Assisted by an LLM (Claude Code)._ -- 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]
