Skip to content
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

Fix Pinot aggregation pushdown for subqueries #12653

Merged
merged 4 commits into from
Apr 19, 2023
Merged

Fix Pinot aggregation pushdown for subqueries #12653

merged 4 commits into from
Apr 19, 2023

Conversation

elonazoulay
Copy link
Member

Description

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

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

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

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 cla-bot bot added the cla-signed label Jun 2, 2022
@elonazoulay elonazoulay changed the title Fix agg pushdown Fix Pinot aggregation pushdown for subqueries Jun 2, 2022
@elonazoulay
Copy link
Member Author

Fixes #12655

@elonazoulay elonazoulay requested a review from hashhar June 2, 2022 18:05
@findepi findepi requested a review from wendigo June 5, 2022 20:27
martint
martint reviewed Jun 14, 2022
View reviewed changes
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotMetadata.java
if (aggregates.size() > 1) {
return false;
}
AggregateFunction aggregate = getOnlyElement(aggregates);
Copy link
Member

Choose a reason for hiding this comment

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

Can aggregates be empty at this point?

Copy link
Member Author

@elonazoulay elonazoulay Mar 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Now aggregates would only contain 1 aggregate.

@martint martint self-requested a review June 14, 2022 18:52
@xiangfu0
Copy link
Contributor

Thanks for fixing this bug!

@elonazoulay elonazoulay requested a review from xiangfu0 June 27, 2022 04:55
@elonazoulay elonazoulay requested a review from xiangfu0 July 19, 2022 00:58
@ellieshen
Copy link

@elonazoulay when can this pull be merged?

@elonazoulay
Copy link
Member Author

Resolves #9134

@elonazoulay elonazoulay mentioned this pull request Mar 30, 2023
Open
@elonazoulay elonazoulay force-pushed the fix_agg_pushdown branch 4 times, most recently from f85aab4 to cb49aa2 Compare April 1, 2023 03:09
kokosing
kokosing reviewed Apr 6, 2023
View reviewed changes
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotMetadata.java Outdated Show resolved Hide resolved
.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java Outdated
@@ -1864,6 +1864,52 @@ public void testAggregationPushdown()
.isThrownBy(() -> query("SELECT bool_col, COUNT(long_col) FROM \"SELECT bool_col, long_col FROM " + ALL_TYPES_TABLE + " GROUP BY bool_col, long_col\""))
.withRootCauseInstanceOf(RuntimeException.class)
.withMessage("Operation not supported for DISTINCT aggregation function");

// Verify that count(<column name>) is pushed down only when it matches a COUNT(DISTINCT <column name>) query
assertThat(query("SELECT COUNT(bool_col) FROM" +
Copy link
Member

Choose a reason for hiding this comment

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

use """

Copy link
Member Author

@elonazoulay elonazoulay Apr 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Done. Noticed that using formatted(...) disables the SQL formatting so the table names are hardcoded. Seems to be similar to other use cases, lmk if you prefer using formatted(tableName).

.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java Outdated Show resolved Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotMetadata.java Outdated Show resolved Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotMetadata.java Outdated
@@ -423,6 +424,44 @@ public static PinotColumnHandle toNonAggregateColumnHandle(PinotColumnHandle col
return new PinotColumnHandle(columnHandle.getColumnName(), columnHandle.getDataType(), quoteIdentifier(columnHandle.getColumnName()), false, false, true, Optional.empty(), Optional.empty());
}

private boolean canPushDownAggregateIfGroupingColumnsPresent(ConnectorSession session, Optional<DynamicTable> dynamicTable, List<AggregateFunction> aggregates, Map<String, ColumnHandle> assignments)
Copy link
Member

Choose a reason for hiding this comment

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

it looks like this method does things that do not match what the name suggests (see pattern matching implementCountDistinct.getPattern().matches()

Copy link
Member Author

Choose a reason for hiding this comment

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

lmk if the new name is ok: isAggregationPushdownSupported. Thanks for all the advice!

@sumit-gupta-sgt
Copy link

sumit-gupta-sgt commented Apr 10, 2023 •
edited
Loading

@elonazoulay @hashhar when can this pull be merged?

@elonazoulay elonazoulay requested a review from kokosing April 14, 2023 18:19
@elonazoulay elonazoulay requested a review from findepi April 16, 2023 03:42
kokosing
kokosing approved these changes Apr 16, 2023
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

% minor comments

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotMetadata.java Show resolved Hide resolved
@github-actions github-actions bot added the docs label Apr 17, 2023
@elonazoulay elonazoulay requested a review from kokosing April 17, 2023 22:40
@kokosing kokosing merged commit 4c0e62c into trinodb:master Apr 19, 2023
@kokosing
Copy link
Member

Thank you!

@github-actions github-actions bot added this to the 414 milestone Apr 19, 2023
@colebow colebow mentioned this pull request Apr 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kokosing kokosing kokosing approved these changes

@hashhar hashhar Awaiting requested review from hashhar

@wendigo wendigo Awaiting requested review from wendigo

@martint martint Awaiting requested review from martint

@xiangfu0 xiangfu0 Awaiting requested review from xiangfu0

@findepi findepi Awaiting requested review from findepi

Assignees
No one assigned
Labels
cla-signed docs
Milestone
414
Development

Successfully merging this pull request may close these issues.
6 participants
@elonazoulay @xiangfu0 @ellieshen @sumit-gupta-sgt @kokosing @martint