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.

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

Fix global empty aggregation pushdown in JDBC connectors #12603

Merged
merged 1 commit into from
May 31, 2022

Conversation

findepi
Copy link
Member

@findepi findepi commented May 31, 2022

In a query like ...FROM (SELECT count(*) FROM t) the aggregation is
global, yet the aggregation itself may be unused and pruned away.

In such case, JDBC connectors ConnectorMetadata.applyAggregation would
produce a query with no aggregation function (as it was pruned away) and
with no GROUP BY, since global aggregation was supposed to be
implicit, so returning original table, without aggregating.

This commit fixes this by avoiding such pushdown on the engine side. In
such case, the engine knows the result without involving the connector.
Additionally, a safety check is added in DefaultJdbcMetadata.

Fixes #12598

@cla-bot cla-bot bot added the cla-signed label May 31, 2022
@findepi findepi force-pushed the findepi/agg-union-all-agg branch from f93d69d to e7c3970 Compare May 31, 2022 09:06
@findepi findepi changed the title Test aggregation over UNION ALL of aggregation Fix global empty aggregation pushdown in JDBC connectors May 31, 2022
@findepi findepi requested review from wendigo and hashhar May 31, 2022 09:07
@findepi findepi force-pushed the findepi/agg-union-all-agg branch from e7c3970 to 641d58a Compare May 31, 2022 09:55
@findepi
Copy link
Member Author

findepi commented May 31, 2022

@wendigo thanks for your quick review. PTAL, i changed the approach.

In a query like `...FROM (SELECT count(*) FROM t)` the aggregation is
global, yet the aggregation itself may be unused and pruned away.

In such case, JDBC connectors `ConnectorMetadata.applyAggregation` would
produce a query with no aggregation function (as it was pruned away) and
with no `GROUP BY`, since global aggregation was supposed to be
implicit, so returning original table, without aggregating.

This commit fixes this by avoiding such pushdown on the engine side. In
such case, the engine knows the result without involving the connector.
Additionally, a safety check is added in `DefaultJdbcMetadata`.
@findepi findepi force-pushed the findepi/agg-union-all-agg branch from 641d58a to 4ed5e21 Compare May 31, 2022 12:34
@@ -138,6 +139,11 @@ public static Optional<PlanNode> pushAggregationIntoTableScan(
LiteralEncoder literalEncoder = new LiteralEncoder(plannerContext);
Session session = context.getSession();

if (groupingKeys.isEmpty() && aggregations.isEmpty()) {
// Global aggregation with no aggregation functions
return Optional.of(new ValuesNode(aggregationNode.getId(), 1));
Copy link
Member Author

Choose a reason for hiding this comment

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

This will be further generalized in #12612

@findepi findepi merged commit 3595a4f into trinodb:master May 31, 2022
@findepi findepi deleted the findepi/agg-union-all-agg branch May 31, 2022 19:51
@findepi findepi mentioned this pull request May 31, 2022
@github-actions github-actions bot added this to the 383 milestone May 31, 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.

Incorrect result in JDBC connectors when aggregation result is pruned away
3 participants