Support pushdown of `count` aggregation with distinct #8562

alexjo2144 · 2021-07-14T21:29:01Z

Moves the distinct aggregation rewriting optimizers to after PushAggregationIntoTableScan.

TODO: I'm not as familiar with testing around PlanOptimizers, is there a good way to test this?

findepi · 2021-07-15T12:00:08Z

TODO: I'm not as familiar with testing around PlanOptimizers, is there a good way to test this?

i am afraid there isn't, since we don't have a mock connector for exercising aggregation pushdown SPI calls.
Unless you create one.

findepi · 2021-07-15T12:01:49Z

Taking these words back. Since you're making ImplementCount support DISTINCT aggregations, you should be able to verify pushdown is happening in io.trino.plugin.jdbc.BaseJdbcConnectorTest#testAggregationPushdown (eg with PostgreSQL)

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/ImplementCount.java

findepi · 2021-07-16T10:58:14Z

@alexjo2144 can you please review build output?

alexjo2144 · 2021-07-16T18:04:24Z

@alexjo2144 can you please review build output?

Yeah, I need to think about this a little more. There are some pushdowns that I'm still not fully understanding.

For example, with the original code SELECT min(DISTINCT regionkey) is fully pushed down as SELECT min("regionkey") AS "tmp" FROM (SELECT "regionkey" FROM "tpch"."nation" GROUP BY "regionkey"), which is a little odd. However, after my changes only the groupby is pushded down, which is definitely not what we want.

core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

plugin/trino-phoenix/src/test/java/io/trino/plugin/phoenix/TestPhoenixConnectorTest.java

core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

losipiuk · 2021-08-26T17:25:52Z

@findepi should be good now (I hope). Please give it another look.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestingH2JdbcClient.java

core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java

findepi · 2021-08-27T13:02:13Z

...in/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/ImplementCountDistinct.java

@@ -0,0 +1,90 @@
+/*


Allows pushdown of multiple distinct aggregations as long as
only one non-count aggregation is distinct.

i may have asked about that but don't remember answer -- why "only one non-count" ?

I am not sure what exactly @alexjo2144 had in mind here.
There are many constraints here. And definitely not all queries which satisfy the condition from commit message will be be fully pushed down

E.g. this one will not:

select max(distinct regionkey), count(distinct regionkey), count(distinct nationkey) from nation;

I suggest to just leave title line in commit message and drop the rest.

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

plugin/trino-phoenix/src/test/java/io/trino/plugin/phoenix/TestPhoenixConnectorTest.java

plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java

findepi · 2021-08-27T13:05:08Z

plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClient.java

@@ -282,8 +283,9 @@ public PostgreSqlClient(
                this::quoted,
                ImmutableSet.<AggregateFunctionRule<JdbcExpression>>builder()
                        .add(new ImplementCountAll(bigintTypeHandle))
-                        .add(new ImplementCount(bigintTypeHandle))


redudantn move

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

hashhar · 2021-08-27T13:38:22Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcConnectorTest.java

@@ -118,18 +117,6 @@ protected TestTable createTableWithUnsupportedColumn()
        return Optional.of(dataMappingTestSetup);
    }

-    @Override
-    protected Optional<DataMappingTestSetup> filterCaseSensitiveDataMappingTestData(DataMappingTestSetup dataMappingTestSetup)


Is this related?

findepi

"core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java"

findepi · 2021-08-30T09:05:59Z

core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java

@@ -627,19 +626,36 @@ public PlanOptimizers(
                .addAll(projectionPushdownRules)
                .add(new PushProjectionIntoTableScan(metadata, typeAnalyzer, scalarStatsCalculator))
                .add(new RemoveRedundantIdentityProjections())
+                .add(new PushLimitThroughMarkDistinct())


add a comment why this is inside pushIntoTableScanRulesExceptJoins set, otherwise looks like unintentional

maybe it shouldn't be here? instead, PushLimitThroughMarkDistinct could come together with MultipleDistinctAggregationToMarkDistinct?

maybe it shouldn't be here? instead, PushLimitThroughMarkDistinct could come together with MultipleDistinctAggregationToMarkDistinct?

per @kasiafi

After removing MultipleDistinctAggregationToMarkDistinct from an IterativeOptimizer, there's no way that a MarkDistinctNode could be present at that stage, so the rule PushLimitThroughMarkDistinct can be also removed. However, I'd keep it there next to other PushLimit rules in case it's needed again in the future. Actually, the PushLimit rules could be extracted like projectionPushdownRules.

never mind. Reworking after chat with @kasiafi

findepi · 2021-08-30T09:07:33Z

core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java

@@ -855,6 +871,7 @@ public PlanOptimizers(
                            // DetermineTableScanNodePartitioning is needed to needs to ensure all table handles have proper partitioning determined
                            // Must run before AddExchanges
                            .add(new DetermineTableScanNodePartitioning(metadata, nodePartitioningManager, taskCountEstimator))
+                            .add(new MultipleDistinctAggregationToMarkDistinct())


We had MultipleDistinctAggregationToMarkDistinct once and now we have twice. What's the reason?

Does not make sense.

losipiuk · 2021-08-30T14:05:53Z

@kasiafi done (hopefully) as we discussed. Much simpler. Thanks. Let's see what CI says.

findepi · 2021-08-30T15:29:26Z

core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java

+                                .add(new PushAggregationThroughOuterJoin())
+                                .add(new ReplaceRedundantJoinWithSource()) // Run this after PredicatePushDown optimizer as it inlines filter constants
+                                .add(new MultipleDistinctAggregationToMarkDistinct()) // Run this after aggregation pushdown
+                                .addAll(limitPushdownRules)


Suggested change

.addAll(limitPushdownRules)

.addAll(limitPushdownRules) // including through MarkDistinct

Actually having this here results in test failures. @kasiafi looked deeply into that and the problem comes from extra call of PushLimitThroughOuterJoin and the fact that applyJoin will not consume same limit twice.
So without this optimization went

... Limit -> OuterJoin -> Scan(no limit) (PushLimitThroughOuterJoin) ... Limit -> OuterJoin -> Limit -> Scan(no limit) (PushLimitIntoTableScan) ... Limit -> OuterJoin -> Scan(with limit) now join is pushable to TS

With this extra line the optimization continues from when it completed previously

... Limit -> OuterJoin -> Scan(with limit) (PushLimitThroughOuterJoin) ... Limit -> OuterJoin -> Limit -> Scan(with limit) (PushLimitIntoTableScan; applyJoin return Optional.empty() because of https://github.com/trinodb/trino/blob/408476e5d4067db61324f773510c27731373b91b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java#L459-L461) ... Limit -> OuterJoin -> Limit -> Scan(with limit) !!!!! join no longer pushable to table scan

This could be addressed with #9056 but I am not super convinced this is the way to go.

@findepi, @kasiafi I suggest we go without limit pushdown here for now. It should not matter. What was to be pushed down should be already.
Do you feel it is good to be merged?

The change in PlanOptimizers looks OK.

findepi · 2021-09-01T10:44:04Z

core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java

+                                .add(new RemoveRedundantIdentityProjections())
+                                .add(new PushAggregationThroughOuterJoin())
+                                .add(new ReplaceRedundantJoinWithSource()) // Run this after PredicatePushDown optimizer as it inlines filter constants
+                                .add(new MultipleDistinctAggregationToMarkDistinct()) // Run this after aggregation pushdown


... so that multiple distinct aggregations can be pushed into a connector

i wonder whether we actually considered solving the problem differently. instead of changing rule order, we could have a rule that matches to MarkDistinct and invokes aggregation pushdown.

trying this out could be a follow up. if successful, we could simply this class, presumably even rolling the changes back

MarkDistinct is not always a result of the MultipleDistinctAggregationToMarkDistinct rule. And when it is, it could be hard to identify the original aggregations because of intervening rules.
Also, applying rule MultipleDistinctAggregationToMarkDistinct later might be possibly beneficial from the viewpoint of decorrelation.

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

plugin/trino-phoenix/src/test/java/io/trino/plugin/phoenix/TestPhoenixConnectorTest.java

plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java

cla-bot bot added the cla-signed label Jul 14, 2021

findepi reviewed Jul 15, 2021

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/ImplementCount.java Outdated Show resolved Hide resolved

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/ImplementCount.java Outdated Show resolved Hide resolved

alexjo2144 force-pushed the pushdown-count-distinct branch 7 times, most recently from 5086bb1 to cca1e07 Compare July 21, 2021 15:49

findepi reviewed Jul 22, 2021

View reviewed changes

findepi mentioned this pull request Jul 26, 2021

Fix incorrect results for aggregation functions on case-sensitive types #8551

Merged

alexjo2144 force-pushed the pushdown-count-distinct branch 3 times, most recently from 75311f9 to 3831de0 Compare July 26, 2021 21:20

findepi force-pushed the master branch from 8538e49 to 1f896ea Compare July 30, 2021 22:13

alexjo2144 force-pushed the pushdown-count-distinct branch 4 times, most recently from 3ef0814 to 950f732 Compare August 13, 2021 18:55

losipiuk force-pushed the pushdown-count-distinct branch 7 times, most recently from 59c5585 to 02581d8 Compare August 18, 2021 12:46

losipiuk force-pushed the pushdown-count-distinct branch from ad90d51 to 32b3974 Compare August 25, 2021 21:52

findepi reviewed Aug 26, 2021

View reviewed changes

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java Outdated Show resolved Hide resolved

losipiuk force-pushed the pushdown-count-distinct branch from 1c6d6f2 to 94e67a3 Compare August 26, 2021 15:38

losipiuk mentioned this pull request Aug 26, 2021

Extract limitPushdownRules #8989

Merged

losipiuk force-pushed the pushdown-count-distinct branch from 94e67a3 to cf2e758 Compare August 26, 2021 17:23

findepi approved these changes Aug 27, 2021

View reviewed changes

hashhar reviewed Aug 27, 2021

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java Outdated Show resolved Hide resolved

hashhar reviewed Aug 27, 2021

View reviewed changes

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java Show resolved Hide resolved

hashhar reviewed Aug 27, 2021

View reviewed changes

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java Show resolved Hide resolved

hashhar reviewed Aug 27, 2021

View reviewed changes

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java Outdated Show resolved Hide resolved

hashhar reviewed Aug 27, 2021

View reviewed changes

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java Outdated Show resolved Hide resolved

hashhar reviewed Aug 27, 2021

View reviewed changes

losipiuk force-pushed the pushdown-count-distinct branch 2 times, most recently from 6d29af0 to 86fa04e Compare August 28, 2021 23:23

findepi mentioned this pull request Aug 30, 2021

Grouping keys may be read by JDBC connector after aggregation pushdown even if not needed by upstream query execution #9021

Closed

findepi reviewed Aug 30, 2021

View reviewed changes

findepi approved these changes Sep 1, 2021

View reviewed changes

losipiuk force-pushed the pushdown-count-distinct branch from e13aba1 to 5d919d9 Compare September 1, 2021 11:31

alexjo2144 and others added 3 commits September 1, 2021 13:32

Include grouping sets in JdbcTableHandle column list

59c6c28

Drop unnecessary braces

d81ee7c

Rename variable

9f304b5

losipiuk force-pushed the pushdown-count-distinct branch from 5d919d9 to 1d9830c Compare September 1, 2021 11:32

Implement count(distinct) pushdown for PostgreSQL

18444e5

losipiuk force-pushed the pushdown-count-distinct branch from 1d9830c to 18444e5 Compare September 2, 2021 09:20

losipiuk merged commit d55bb1c into trinodb:master Sep 2, 2021

ebyhr mentioned this pull request Sep 9, 2021

Incorrect results of COUNT(DISTINCT) queries in MemSQL connector #9166

Closed

ebyhr added this to the 362 milestone Sep 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support pushdown of `count` aggregation with distinct #8562

Support pushdown of `count` aggregation with distinct #8562

alexjo2144 commented Jul 14, 2021 •

edited

Loading

findepi commented Jul 15, 2021

findepi commented Jul 15, 2021

findepi commented Jul 16, 2021

alexjo2144 commented Jul 16, 2021

losipiuk commented Aug 26, 2021

findepi Aug 27, 2021

losipiuk Aug 28, 2021

findepi Aug 27, 2021

hashhar Aug 27, 2021

findepi left a comment

findepi Aug 30, 2021

losipiuk Aug 30, 2021

losipiuk Aug 30, 2021

findepi Aug 30, 2021

losipiuk Aug 30, 2021

losipiuk commented Aug 30, 2021

findepi Aug 30, 2021

losipiuk Aug 31, 2021 •

edited

Loading

losipiuk Aug 31, 2021

kasiafi Sep 1, 2021

findepi Sep 1, 2021

findepi Sep 1, 2021

kasiafi Sep 1, 2021

	.addAll(limitPushdownRules)
	.addAll(limitPushdownRules) // including through MarkDistinct

Support pushdown of count aggregation with distinct #8562

Support pushdown of count aggregation with distinct #8562

Conversation

alexjo2144 commented Jul 14, 2021 • edited Loading

findepi commented Jul 15, 2021

findepi commented Jul 15, 2021

findepi commented Jul 16, 2021

alexjo2144 commented Jul 16, 2021

losipiuk commented Aug 26, 2021

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

findepi 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

losipiuk commented Aug 30, 2021

Choose a reason for hiding this comment

losipiuk Aug 31, 2021 • edited Loading

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

Support pushdown of `count` aggregation with distinct #8562

Support pushdown of `count` aggregation with distinct #8562

alexjo2144 commented Jul 14, 2021 •

edited

Loading

losipiuk Aug 31, 2021 •

edited

Loading