andygrove opened a new pull request, #7286: URL: https://github.com/apache/datafusion-comet/pull/7286
## Which issue does this PR close? Backport of #7222 to `branch-1.2`. #7222 closes #6880 on `main`. Part of #7204. ## Rationale for this change Since #6867, a native `PartialMerge` runs as a single-mode aggregate, which DataFusion 55.2 cannot spill when its input is sorted on a prefix of the grouping keys. A `count(DISTINCT k)` over input sorted on the group key then fails the task with `Additional allocation failed for GroupedHashAggregateStream`. #7222 runs such a `PartialMerge` as a final aggregate, which does have a spill path. `branch-1.2` has the failure, so it needs the fix before 1.2.0 ships. `branch-1.1` also has #6867, through #7020, so it has the same failure. Its backport is not part of this PR. ## What changes are included in this PR? `git cherry-pick -x d8ead26b1e` of the squash commit of #7222. The planner changes in `planner.rs`, `planner/aggregate.rs` and `merge_as_partial.rs` applied without conflicts. Adaptations: - Dropped the comment on `limited_to_its_share` in `fair_pool.rs`, the comment on `ORDERED_FINAL_AGGREGATE` in `spill_replay.rs`, and the paragraph in `memory_management.md`. They describe the `OrderedFinalAggregateStream` exemption from the fair limit, which came in with #7096 and is not on this branch. The header comment in `spill_replay.rs` applies as it is. - The sort-merge join test uses a 32 MiB pool instead of 16 MiB. Without #7096 a consumer's fair limit is the pool divided by the number of consumers, and at 16 MiB the sort below the join fails with `Not enough memory to continue external sort` before the `PartialMerge` runs. At 32 MiB the `PartialMerge` spills, and the test still asserts that it does. ## How are these changes tested? On this branch, with a `libcomet` built from the pick: - `cargo check --all-targets` in `native/` passes, and the new Rust test `partial_merge_over_sorted_input_runs_as_final` passes. - All of `CometAggregateSuite` passes: 164 tests, none failed. - Against a `libcomet` built from unmodified `branch-1.2`, both new tests fail with `Additional allocation failed for GroupedHashAggregateStream`. So the tests catch the bug, and the branch has it. I tested the pick before #7222 merged, then re-picked the squash commit. The re-pick has the same content, and `cargo check --all-targets` passes on it. I did not rerun the JVM suites on the re-pick, and I did not run the other suites that #7222 ran. -- 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]
