andygrove commented on PR #4932: URL: https://github.com/apache/datafusion-comet/pull/4932#issuecomment-5169646368
Thanks for the revisions. Both points from last round are handled well. The defensive `clear()` calls are in place on all three paths now, and `repeated_merges_are_within_bound` folding three summaries is exactly the coverage I was hoping for. I wanted to be confident the in-place merge did not perturb results, so I built a differential harness that fingerprints `to_bytes()` plus a 7-percentile query, and ran matching scenarios on `main` and on this branch. Sequential folds crossing the 50000 head-flush boundary, reversed merge order to hit the non-commutative interleave, and a duplicate-heavy case all produce identical bytes. That part looks solid. One thing I would like to settle before merging. Since `groups_accumulator_supported` is `false`, grouped aggregation holds one `QuantileSummaries` per group through `GroupsAccumulatorAdapter`. Two allocations that `main` freed are now retained for the accumulator's lifetime. `head_sampled` keeps its capacity across flushes, where `main` dropped it at the `mem::take`. And `sampled_buffer` keeps the capacity from the last merge. The second one is amplified by `res.reserve(current_samples.len())` in `compress_immut`, which sizes the output buffer to the uncompressed merged length even though the compressed result is much smaller. After a merge both `sampled` and `sampled_buffer` end up near the merged length rather than the compressed length. `heap_size()` accounts for all of it, so the memory tracking stays truthful, which is good. My worry is peak footprint on a wide group-by, where doubling per-group bytes could push us into spilling and cost more than the allocation savings gain. Could you run `CometAggregateExpressionBenchmark` before and after and share wall-clock numbers, particularly `approx_percentile_double_high_card` alongside `approx_percentile_double_median`? The `PR Benchmark Check` job was skipped here, so we have allocation volume but no timing evidence yet. If the high-cardinality case does regress, one option is to drop the `reserve` inside `compress_immut` and let the output grow amortized, since the compressed length is bounded by the compress threshold rather than by the merged input length. Would also be worth a test that asserts `heap_size()` stays bounded across many repeated merges, so the retention cannot quietly grow in a later change. _Review assisted by an LLM._ -- 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]
