jayzhan211 commented on PR #26703: URL: https://github.com/apache/datafusion/pull/26703#issuecomment-6703678538
@Jefffrey thanks for the review. This PR changed substantially after your approval, so here is what's new. Your 8 reviewed commits are unchanged apart from a rebase, and the last 3 commits are new. I plan to merge it; if you'd like to look, `next_batch` in `aggregate_hash_table/common.rs` is the part you haven't seen, and I'll follow up in a new PR on anything you find. - **Aggregate output:** `next_batch` now gives a slice with more values than rows only the values its rows reference. This fixes #26696 at its source, so it also helps the repartition coalescer and the final aggregate. ClickBench partitioned with dictionaries goes from 78.8 s to 42.8 s, and peak memory from 13.1 to 12.7 GiB ([bot run](https://github.com/apache/datafusion/pull/26703#issuecomment-6703599262)). - **Compaction:** the new `compact_dictionary` takes each referenced value once. Copying a value per row, as the approved version did, gave wrong hashes and function results when a dictionary had more rows than its key type holds, could fail with an offset overflow, and was over 100x slower than main for a repeated 64 KB value. - **Threshold:** `MAX_DICTIONARY_VALUES_PER_ROW` goes from 16 to 64, because compacting just above 16 made cheap functions such as `lower` slower. It is now only a guard for filters and scans, where I measured no gain. - **Tests:** the `dictionary.slt` comparison never reached oversized dictionaries, because a filter on the group key is pushed below the aggregate. It is now projected, with a new block for filter output, and the bugs above have unit tests. `compact_dictionary` is new public API, next to `map_dictionary_values` and `MAX_DICTIONARY_VALUES_PER_ROW`. -- 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]
