viirya commented on PR #26782: URL: https://github.com/apache/datafusion/pull/26782#issuecomment-6510794881
Thanks @alamb for the review and the ablation. @comphead, thanks for the careful review. I pushed 32b641fb7: - The `take_state_batch` gap is fixed for both the clustered and the non-clustered hash tables (details inline). - Dropped `groups.shrink_to_fit()` and `states.shrink_to_fit()`. You're right that they are no-ops on these paths: with full clustering, `split_vec_min_alloc` already moves the active group into a fresh allocation, and `take_state_batch` emits `EmitTo::All`. - Added the `free_scratch()` helper and consolidated the tests. On the design questions: - Chunked hashing in `HllGroupsAccumulator` sounds good as a follow-up: it would bound `hashes` regardless of batch size and avoid regrowing it under sustained pressure. The hook is still needed for `GroupsAccumulatorAdapter`, whose per-batch indices also scale with the batch. - Shrinking before giving up: confirmed. With three 8192-row batches of a single key, the partial stream fails a 64 KiB limit asking for 132.1 KB, but the table is about 4.3 KB after `shrink_to_fit`. The final and single streams have their own no-completed-groups paths (spill or error) that would change too, so I'd like to handle all three in a follow-up: #26911. -- 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]
