Add Exchange before GroupId to improve Partial Aggregation #24047

aaneja · 2024-11-14T12:07:34Z

See #23475 for more details

Previously closed PR - #11741

Description

Motivation and Context

See Javadoc of the new AddExchangesBelowPartialAggregationOverGroupIdRuleSet

Impact

Better performance for TPCDS Q22, Q67
See plan diffs (TPCDS SF 1000, unpartitioned) - https://aaneja.github.io/mypages/PR_24047_AddExchangesBelowPartialAggregationOverGroupId_OffVsOn.html

Test Plan

TODO : Add a new planner test

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Added a new optimizer rule to add exchanges below a combination of partial aggregation+ GroupId . Enabled with the boolean session property `enable_forced_exchange_below_group_id`

steveburnett · 2024-11-14T14:30:20Z

Thanks for the release note entry! Minor formatting nits, and include the PR number.

== RELEASE NOTES ==

General Changes
* Add a new optimizer rule to add exchanges below a combination of partial aggregation+ GroupId. Enabled with the boolean session property ``enable_forced_exchange_below_group_id``. :pr:`24047`

ZacBlanco

high level first pass. Seems good for the most part. I will take another pass and look at the details of the rule tomorrow.

presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java

...presto/sql/planner/iterative/rule/AddExchangesBelowPartialAggregationOverGroupIdRuleSet.java

...ook/presto/sql/planner/TestLogicalAddExchangesBelowPartialAggregationOverGroupIdRuleSet.java

...presto/sql/planner/iterative/rule/AddExchangesBelowPartialAggregationOverGroupIdRuleSet.java

rschlussel · 2025-01-22T19:27:09Z

...presto/sql/planner/iterative/rule/AddExchangesBelowPartialAggregationOverGroupIdRuleSet.java

+                return false;
+            }
+
+            return isEnabledAddExchangeBelowGroupId(session);


maybe this should have 3 possible values - ALWAYS, COST_BASED, and NEVER (similar to partial aggregation pushdown). that way someone can enable this if they don't have stats or if the stats estimates are no good.

What would we re-partition on if ALWAYS is chosen (for the non-trivial case of more than one partition variable) ?

good point. i guess we can leave as is for now, unless we want to has on all of them?

I'm in favor of leaving this as-is until a use case arises for ALWAYS

...to/sql/planner/iterative/rule/TestAddExchangesBelowPartialAggregationOverGroupIdRuleSet.java

rschlussel · 2025-01-28T17:37:49Z

...to/sql/planner/iterative/rule/TestAddExchangesBelowPartialAggregationOverGroupIdRuleSet.java

+    }
+
+    @Test
+    public void testAddExchangesWithoutProjection()


what about a withProjection test. Also a test that it doesn't fire if it's disabled, only has one grouping set, has pass through keys.

'only has one grouping set' , added with this test

withProjection, does not fire if disabled -> will add

only has one grouping set, has pass through keys -> I could not build a use-case where this occurs. My understanding of when this could occur is unclear. Can you help me out with an example ?

sorry, i didn't mean one grouping set with pass through keys, i meant the case covered by https://github.com/prestodb/presto/pull/24047/files#diff-2e788a27c31ea3e4d5d404dba18eee33a3868d656bae3b0c0380269afb838f24, so multiple grouping sets that all share some common keys.

@rschlussel I think I've covered all the test cases now. Please take a look

steveburnett · 2025-02-03T15:05:37Z

Thanks for the release note entry! Minor formatting nits, and include the PR number.

== RELEASE NOTES ==

General Changes
* Add a new optimizer rule to add exchanges below a combination of partial aggregation+ GroupId. Enabled with the boolean session property ``enable_forced_exchange_below_group_id``.

The minor formatting nits should still apply, but new release note guidelines as of last week: PR #24354 automatically adds links to this PR to the release notes. Please remove the manual PR link in the following format from the release note entries for this PR.

:pr:`12345`

I have updated the Release Notes Guidelines to remove the examples of manually adding the PR link.

aaneja · 2025-02-04T04:56:01Z

@steveburnett Thanks for the feedback! We're still working through some naming+defaults for the new session/feature flags. I will update the release notes + optimizer docs once we close on this

ZacBlanco

lgtm. one minor thing

...presto/sql/planner/iterative/rule/AddExchangesBelowPartialAggregationOverGroupIdRuleSet.java

aaneja · 2025-02-07T15:17:30Z

presto-main/src/main/java/com/facebook/presto/sql/planner/plan/GroupIdNode.java

@@ -81,7 +81,9 @@ public GroupIdNode(
    {
        super(sourceLocation, id, statsEquivalentPlanNode);
        this.source = requireNonNull(source);
-        this.groupingSets = listOfListsCopy(requireNonNull(groupingSets, "groupingSets is null"));
+        checkArgument(requireNonNull(groupingSets, "groupingSets is null").size() > 1,


I noticed that we only create a GroupId node iff the size of the grouping sets is >1. So adding this invariant here, made sense. No tests failed, so IMO this should be added cc: @rschlussel

Based on: trinodb/trino@dc1d66fb co-authored-by: Piotr Findeisen <piotr.findeisen@gmail.com> Based on : trinodb/trino@c573b34 co-authored-by: Lukasz Stec <lukasz.stec@starburstdata.com> Based on: trinodb/trino@29328d3 co-authored-by: praveenkrishna <praveenkrishna@tutanota.com>

prestodb-ci added the from:IBM PR from IBM label Nov 14, 2024

aaneja force-pushed the groupIdExchangeOptimization branch from 95fb49c to 62aaab6 Compare November 14, 2024 12:13

aaneja force-pushed the groupIdExchangeOptimization branch from 62aaab6 to fa61dfd Compare November 18, 2024 12:58

aaneja force-pushed the groupIdExchangeOptimization branch from fa61dfd to 39222b3 Compare January 9, 2025 17:03

aaneja force-pushed the groupIdExchangeOptimization branch 2 times, most recently from 86b145a to 5868a2f Compare January 21, 2025 14:34

aaneja marked this pull request as ready for review January 21, 2025 14:35

aaneja requested review from jaystarshot, feilong-liu, elharo, ClarenceThreepwood and a team as code owners January 21, 2025 14:35

aaneja requested review from presto-oss and rschlussel January 21, 2025 14:35

ZacBlanco reviewed Jan 22, 2025

View reviewed changes

aaneja added a commit to aaneja/mypages that referenced this pull request Jan 24, 2025

Plan diff for prestodb/presto#24047

605bfd6

aaneja force-pushed the groupIdExchangeOptimization branch from 5868a2f to 437a09a Compare January 24, 2025 05:16

rschlussel reviewed Jan 28, 2025

View reviewed changes

aaneja force-pushed the groupIdExchangeOptimization branch from 437a09a to 979d204 Compare February 3, 2025 12:47

aaneja force-pushed the groupIdExchangeOptimization branch 2 times, most recently from 0874d82 to f95fab9 Compare February 4, 2025 06:13

ZacBlanco previously approved these changes Feb 4, 2025

View reviewed changes

...presto/sql/planner/iterative/rule/AddExchangesBelowPartialAggregationOverGroupIdRuleSet.java Outdated Show resolved Hide resolved

aaneja dismissed ZacBlanco’s stale review via dcf0800 February 6, 2025 16:22

aaneja force-pushed the groupIdExchangeOptimization branch 4 times, most recently from e196f21 to 74178b4 Compare February 7, 2025 13:21

aaneja commented Feb 7, 2025

View reviewed changes

aaneja force-pushed the groupIdExchangeOptimization branch from 74178b4 to 84cafec Compare February 7, 2025 15:18

rschlussel approved these changes Feb 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Exchange before GroupId to improve Partial Aggregation #24047

Add Exchange before GroupId to improve Partial Aggregation #24047

aaneja commented Nov 14, 2024 •

edited

Loading

steveburnett commented Nov 14, 2024

ZacBlanco left a comment

rschlussel Jan 22, 2025

aaneja Feb 3, 2025

rschlussel Feb 5, 2025

aaneja Feb 6, 2025

rschlussel Jan 28, 2025

aaneja Feb 3, 2025

rschlussel Feb 5, 2025

aaneja Feb 7, 2025

steveburnett commented Feb 3, 2025

aaneja commented Feb 4, 2025

ZacBlanco left a comment

aaneja Feb 7, 2025

Add Exchange before GroupId to improve Partial Aggregation #24047

Are you sure you want to change the base?

Add Exchange before GroupId to improve Partial Aggregation #24047

Conversation

aaneja commented Nov 14, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

steveburnett commented Nov 14, 2024

ZacBlanco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveburnett commented Feb 3, 2025

aaneja commented Feb 4, 2025

ZacBlanco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaneja commented Nov 14, 2024 •

edited

Loading