New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor AggregationNode creation #12101

Merged

sopel39 merged 4 commits into trinodb:master from starburstdata:ls/013-aggregation-node-builder

May 27, 2022

Member

lukasz-stec commented Apr 22, 2022

Description

This is a refactoring that extracts multiple ways AggregationNode is being instantiated to helper methods + AggregationNodeBuilder when applicable.
This reduces code duplication and makes it easier to understand the code intent.
Also, it makes it easier to modify AggregationNode e.g. add new fields.

Is this change a fix, improvement, new feature, refactoring, or other?

refactoring

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

core query engine

How would you describe this change to a non-technical end user or system administrator?

Code refactoring

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

cla-bot bot added the cla-signed label

lukasz-stec requested a review from sopel39

April 22, 2022 13:21

lukasz-stec force-pushed the ls/013-aggregation-node-builder branch 2 times, most recently from 071b30f to a676ced Compare

April 23, 2022 17:37

Member Author

lukasz-stec commented Apr 25, 2022

The only CI failure is a flaky test #11275

lukasz-stec force-pushed the ls/013-aggregation-node-builder branch from a676ced to 682c0a9 Compare

April 25, 2022 07:51

Member Author

lukasz-stec commented Apr 25, 2022

replaced AggregationNode#distinctAggregation with AggregationNode#restoreDistinctAggregation

lukasz-stec mentioned this pull request

Use "union" in partial aggregation output #12155

Closed

sopel39 reviewed

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java Outdated Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java Outdated Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNodeBuilder.java Outdated Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNodeBuilder.java Outdated Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNodeBuilder.java Outdated Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNodeBuilder.java Outdated Show resolved Hide resolved

lukasz-stec force-pushed the ls/013-aggregation-node-builder branch from 682c0a9 to 50fa940 Compare

May 24, 2022 07:52

lukasz-stec commented

View reviewed changes

Member Author

lukasz-stec left a comment

comments addressed

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java Outdated Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java Outdated Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNodeBuilder.java Outdated Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNodeBuilder.java Outdated Show resolved Hide resolved

lukasz-stec requested a review from sopel39

May 24, 2022 07:52

sopel39 reviewed

View reviewed changes

Member

sopel39 left a comment

comments

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java Outdated Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/AggregationDecorrelation.java Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/AggregationDecorrelation.java Show resolved Hide resolved

...ain/src/main/java/io/trino/sql/planner/iterative/rule/PushPartialAggregationThroughJoin.java Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/optimizations/PredicatePushDown.java Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java Outdated Show resolved Hide resolved

lukasz-stec force-pushed the ls/013-aggregation-node-builder branch from 50fa940 to 2f170a8 Compare

May 24, 2022 19:46

lukasz-stec commented

View reviewed changes

Member Author

lukasz-stec left a comment

comments addressed

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java Outdated Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/AggregationDecorrelation.java Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/AggregationDecorrelation.java Show resolved Hide resolved

lukasz-stec requested a review from sopel39

May 24, 2022 19:46

lukasz-stec force-pushed the ls/013-aggregation-node-builder branch 2 times, most recently from c8c9683 to 2f3cffb Compare

May 26, 2022 13:16

sopel39 approved these changes

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/planner/LogicalPlanner.java Outdated Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/QueryPlanner.java Outdated Show resolved Hide resolved

Member

sopel39 commented May 26, 2022

small comment

lukasz-stec added 3 commits

May 26, 2022 16:56


          Extract AggregationNode.singleAggregation

ebde19e


          Extract AggregationDecorrelation.restoreDistinctAggregation

a8f95e8


          Fix formatting

1a95038

lukasz-stec force-pushed the ls/013-aggregation-node-builder branch from 2f3cffb to 2602172 Compare

May 26, 2022 14:58

Member Author

lukasz-stec commented May 26, 2022

replaced AggregationNode.singleAggregation with static import + rebased on the master


          Add AggregationNodeBuilder

494c8b8

AggregationNode is created in multiple places
base on existing AggregationNode with some
fields changed.
AggregationNode.Builder makes that semantics
explicit plus it makes it easier to add new
fields to AggregationNode.

lukasz-stec force-pushed the ls/013-aggregation-node-builder branch from 2602172 to 494c8b8 Compare

May 26, 2022 15:02

sopel39 approved these changes

View reviewed changes

sopel39 merged commit a262611 into trinodb:master

github-actions bot added this to the 383 milestone

colebow mentioned this pull request

Add Trino 383 release notes #12565

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels