Skip to content

Add AggregationNode.hasSingleGlobalAggregation#14559

Merged
sopel39 merged 1 commit intotrinodb:masterfrom
starburstdata:ls/047-is-global-aggregation
Oct 12, 2022
Merged

Add AggregationNode.hasSingleGlobalAggregation#14559
sopel39 merged 1 commit intotrinodb:masterfrom
starburstdata:ls/047-is-global-aggregation

Conversation

@lukasz-stec
Copy link
Copy Markdown
Member

Description

Extract single global aggregation (GROUP BY ()) from multiple places to a method in AggregationNode

Non-technical explanation

Code refactoring

Release notes

( X) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

Copy link
Copy Markdown
Member

@raunaqmorarka raunaqmorarka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, @martint @sopel39 can you also PTAL ?

Copy link
Copy Markdown
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm % comment

@lukasz-stec lukasz-stec force-pushed the ls/047-is-global-aggregation branch from a24f8b0 to 90a4e2a Compare October 11, 2022 11:25
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove Returns {@code false} case.

I wondder why

even if all are empty e.g. GROUP BY GROUPING SETS ((), ()).

is relevant

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some places look only at getGroupingKeys().isEmpty() and consider this a global aggregation. This includes any number of empty grouping sets.
This method only allows for a single empty grouping set and I wanted to emphasize that in the comment so it's easier to spot the difference.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which places do that? That would be a bug.

There's only one possible interpretation of "single global aggregation". It must collapse all rows into a single group and produce a single row. The presence of multiple groups would immediately disqualify it as a "single" aggregation.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which places do that?

The most prominent example:

io.trino.sql.planner.LocalExecutionPlanner.Visitor#visitAggregation
...
         if (node.getGroupingKeys().isEmpty()) {
                return planGlobalAggregation(node, source, context);
            }

@lukasz-stec lukasz-stec changed the title Add AggregationNode.isSingleGlobalAggregation Add AggregationNode.hasSingleGlobalAggregation Oct 11, 2022
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be documented using @return in the javadocs:

@return true if the aggregation collapses all rows into a single global group (e.g., as a result of a GROUP BY () query). Otherwise, false.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the comment

@lukasz-stec lukasz-stec force-pushed the ls/047-is-global-aggregation branch from 90a4e2a to d7aff06 Compare October 11, 2022 16:07
@lukasz-stec
Copy link
Copy Markdown
Member Author

flaky TestParquetReader #13633

@lukasz-stec lukasz-stec force-pushed the ls/047-is-global-aggregation branch from d7aff06 to ba65a86 Compare October 12, 2022 06:47
@lukasz-stec
Copy link
Copy Markdown
Member Author

rebased on the master to restart CI

@sopel39 sopel39 merged commit a844763 into trinodb:master Oct 12, 2022
@github-actions github-actions bot added this to the 400 milestone Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants