andygrove commented on PR #4932: URL: https://github.com/apache/datafusion-comet/pull/4932#issuecomment-5007335791
Thanks for this, the buffer reuse is a clean win and the before/after allocation flamegraphs make the impact easy to see. I traced the in-place `merge` against the old return-value version and the behavior looks preserved, including the non-commutative operand order and the memory accounting via `heap_size`. Two small suggestions, neither a blocker. **1. Defensive `clear()` for consistency** `compress_immut` defensively clears its output buffer before filling it, but `with_head_buffer_inserted` and `merge` take `sampled_buffer` and only `reserve` before pushing: ```rust let mut new_samples = std::mem::take(&mut self.sampled_buffer); new_samples.reserve(self.sampled.len() + sorted.len()); ``` This is correct today because `sampled_buffer` is always empty on entry. It does leave a bit of a trap though. If that invariant ever slips in a future change, this path would silently append the stale elements (it pushes after `reserve`) and corrupt the result rather than fail loudly. A `new_samples.clear()` right after the take is essentially free and keeps this consistent with `compress_immut`. Same thought for `merged_sampled` in `merge`. **2. A multi-merge test** The new `flush_and_merge_reuse_sampled_buffers` test locks in the pointer swap, which is great. `merge_is_within_bound` only does a single merge though, and `merge_batch` folds many digests through the reused buffers in sequence. Would you consider adding a test that merges three or more summaries in place and checks the query result against the exact percentile? That would exercise the buffer reuse across repeated merges, which is exactly where a reuse bug would surface. _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]
