You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "asolimando (via GitHub)" <gi...@apache.org> on 2025/08/06 09:55:00 UTC
[PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
asolimando opened a new pull request, #4494:
URL: https://github.com/apache/calcite/pull/4494
See CALCITE-7125.
I have kept the tests as a separate commit to make review easier.
Tests:
- https://github.com/apache/calcite/pull/4246#issuecomment-2794733253 following this comment, I have moved the `JdbcTest` and added two tests in `set-op.iq` to show-case the two rules over the same query
- Adapted `RelOptRuleTests` to cover both variants
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #4494:
URL: https://github.com/apache/calcite/pull/4494#discussion_r2259241783
##########
core/src/main/java/org/apache/calcite/rel/rules/IntersectToDistinctRule.java:
##########
@@ -117,7 +134,7 @@ public IntersectToDistinctRule(Class<? extends Intersect> intersectClass,
for (int i = 0; i < branchCount; i++) {
filters.add(
relBuilder.greaterThan(relBuilder.field(oriFieldCount + i),
- relBuilder.literal(0)));
+ relBuilder.literal(0)));
Review Comment:
Why is there indentation here, although it is irrelevant
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4494:
URL: https://github.com/apache/calcite/pull/4494#discussion_r2259900702
##########
core/src/main/java/org/apache/calcite/rel/rules/IntersectToDistinctRule.java:
##########
@@ -126,12 +143,86 @@ public IntersectToDistinctRule(Class<? extends Intersect> intersectClass,
call.transformTo(relBuilder.build());
}
+ /**
+ * Variant performing a partial aggregation pushdown.
+ *
+ * <p>Original query:
+ * <pre>{@code
+ * SELECT job FROM "scott".emp WHERE deptno = 10
+ * INTERSECT
+ * SELECT job FROM "scott".emp WHERE deptno = 20
+ * }</pre>
+ *
+ * <p>Query after conversion:
+ * <pre>{@code
+ * SELECT job
+ * FROM (
+ * SELECT job, SUM(c)
Review Comment:
Good catch! I went again through all the comments and Javadoc for the original code and improved it to make it match the code more accurately and eliminating the only Hive-related nomenclature still left (`R3` as the relation).
Note: I had to fix formatting as changing the comment made autostyle consider the code as changed too and there was a violation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4494:
URL: https://github.com/apache/calcite/pull/4494#discussion_r2259491475
##########
core/src/main/java/org/apache/calcite/rel/rules/IntersectToDistinctRule.java:
##########
@@ -117,7 +134,7 @@ public IntersectToDistinctRule(Class<? extends Intersect> intersectClass,
for (int i = 0; i < branchCount; i++) {
filters.add(
relBuilder.greaterThan(relBuilder.field(oriFieldCount + i),
- relBuilder.literal(0)));
+ relBuilder.literal(0)));
Review Comment:
Thanks for pointing that out, my IDE reformatted it that way when pasting and I didn't notice!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4494:
URL: https://github.com/apache/calcite/pull/4494#discussion_r2259900702
##########
core/src/main/java/org/apache/calcite/rel/rules/IntersectToDistinctRule.java:
##########
@@ -126,12 +143,86 @@ public IntersectToDistinctRule(Class<? extends Intersect> intersectClass,
call.transformTo(relBuilder.build());
}
+ /**
+ * Variant performing a partial aggregation pushdown.
+ *
+ * <p>Original query:
+ * <pre>{@code
+ * SELECT job FROM "scott".emp WHERE deptno = 10
+ * INTERSECT
+ * SELECT job FROM "scott".emp WHERE deptno = 20
+ * }</pre>
+ *
+ * <p>Query after conversion:
+ * <pre>{@code
+ * SELECT job
+ * FROM (
+ * SELECT job, SUM(c)
Review Comment:
Good catch! I went again through all the comments and Javadoc for the original code and improved it to make it match the code more accurately and eliminating the only Hive-related nomenclature still left (`R3` as the relation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4494:
URL: https://github.com/apache/calcite/pull/4494#issuecomment-3159512892
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4494) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4494&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4494&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4494&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4494&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4494&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4494)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4494:
URL: https://github.com/apache/calcite/pull/4494#discussion_r2259497506
##########
core/src/main/java/org/apache/calcite/rel/rules/IntersectToDistinctRule.java:
##########
@@ -117,7 +134,7 @@ public IntersectToDistinctRule(Class<? extends Intersect> intersectClass,
for (int i = 0; i < branchCount; i++) {
filters.add(
relBuilder.greaterThan(relBuilder.field(oriFieldCount + i),
- relBuilder.literal(0)));
+ relBuilder.literal(0)));
Review Comment:
But I think the original version ([f654ff4](https://github.com/apache/calcite/pull/4494/commits/f654ff4b2acf6955e4aed46e37ff453a6933b5e5)) of the indentation modification is reasonable. There is no need to change it back. I probably didn't adjust the indentation in the original rule correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
Posted by "silundong (via GitHub)" <gi...@apache.org>.
silundong commented on code in PR #4494:
URL: https://github.com/apache/calcite/pull/4494#discussion_r2259554196
##########
core/src/main/java/org/apache/calcite/rel/rules/IntersectToDistinctRule.java:
##########
@@ -126,12 +143,86 @@ public IntersectToDistinctRule(Class<? extends Intersect> intersectClass,
call.transformTo(relBuilder.build());
}
+ /**
+ * Variant performing a partial aggregation pushdown.
+ *
+ * <p>Original query:
+ * <pre>{@code
+ * SELECT job FROM "scott".emp WHERE deptno = 10
+ * INTERSECT
+ * SELECT job FROM "scott".emp WHERE deptno = 20
+ * }</pre>
+ *
+ * <p>Query after conversion:
+ * <pre>{@code
+ * SELECT job
+ * FROM (
+ * SELECT job, SUM(c)
+ * FROM (
+ * SELECT job, COUNT(*) AS c FROM "scott".emp
+ * WHERE deptno = 10 GROUP BY job
+ * UNION ALL
+ * SELECT job, COUNT(*) AS c FROM "scott".emp
+ * WHERE deptno = 20 GROUP BY job)
+ * GROUP BY job)
+ * WHERE c = 2
+ * }</pre>
+ */
+ public void onMatchAggregatePushdown(RelOptRuleCall call) {
+ final Intersect intersect = call.rel(0);
+ if (intersect.all) {
+ return; // nothing we can do
+ }
+ final RelOptCluster cluster = intersect.getCluster();
+ final RexBuilder rexBuilder = cluster.getRexBuilder();
+ final RelBuilder relBuilder = call.builder();
+
+ // 1st level aggregate: create an aggregate(col0, col1, count() as c) for each branch
+ for (RelNode input : intersect.getInputs()) {
+ relBuilder.push(input);
+ relBuilder.aggregate(relBuilder.groupKey(relBuilder.fields()),
+ relBuilder.countStar(null));
Review Comment:
What is the purpose of `count` here? It seems that just `group by` is enough.
##########
core/src/main/java/org/apache/calcite/rel/rules/IntersectToDistinctRule.java:
##########
@@ -126,12 +143,86 @@ public IntersectToDistinctRule(Class<? extends Intersect> intersectClass,
call.transformTo(relBuilder.build());
}
+ /**
+ * Variant performing a partial aggregation pushdown.
+ *
+ * <p>Original query:
+ * <pre>{@code
+ * SELECT job FROM "scott".emp WHERE deptno = 10
+ * INTERSECT
+ * SELECT job FROM "scott".emp WHERE deptno = 20
+ * }</pre>
+ *
+ * <p>Query after conversion:
+ * <pre>{@code
+ * SELECT job
+ * FROM (
+ * SELECT job, SUM(c)
Review Comment:
May be COUNT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando merged PR #4494:
URL: https://github.com/apache/calcite/pull/4494
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #4494:
URL: https://github.com/apache/calcite/pull/4494#issuecomment-3159146931
Thank you very much for finding these issues and fixing them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4494:
URL: https://github.com/apache/calcite/pull/4494#discussion_r2259794264
##########
core/src/main/java/org/apache/calcite/rel/rules/IntersectToDistinctRule.java:
##########
@@ -126,12 +143,86 @@ public IntersectToDistinctRule(Class<? extends Intersect> intersectClass,
call.transformTo(relBuilder.build());
}
+ /**
+ * Variant performing a partial aggregation pushdown.
+ *
+ * <p>Original query:
+ * <pre>{@code
+ * SELECT job FROM "scott".emp WHERE deptno = 10
+ * INTERSECT
+ * SELECT job FROM "scott".emp WHERE deptno = 20
+ * }</pre>
+ *
+ * <p>Query after conversion:
+ * <pre>{@code
+ * SELECT job
+ * FROM (
+ * SELECT job, SUM(c)
+ * FROM (
+ * SELECT job, COUNT(*) AS c FROM "scott".emp
+ * WHERE deptno = 10 GROUP BY job
+ * UNION ALL
+ * SELECT job, COUNT(*) AS c FROM "scott".emp
+ * WHERE deptno = 20 GROUP BY job)
+ * GROUP BY job)
+ * WHERE c = 2
+ * }</pre>
+ */
+ public void onMatchAggregatePushdown(RelOptRuleCall call) {
+ final Intersect intersect = call.rel(0);
+ if (intersect.all) {
+ return; // nothing we can do
+ }
+ final RelOptCluster cluster = intersect.getCluster();
+ final RexBuilder rexBuilder = cluster.getRexBuilder();
+ final RelBuilder relBuilder = call.builder();
+
+ // 1st level aggregate: create an aggregate(col0, col1, count() as c) for each branch
+ for (RelNode input : intersect.getInputs()) {
+ relBuilder.push(input);
+ relBuilder.aggregate(relBuilder.groupKey(relBuilder.fields()),
+ relBuilder.countStar(null));
Review Comment:
I haven't re-checked the logic of the original rule as it's out of scope for the current PR (removing the plan regression introduced by CALCITE-6893), so it's possible that there are further improvements that can be made, but this should be discussed in a new, dedicated ticket.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4494:
URL: https://github.com/apache/calcite/pull/4494#discussion_r2259497506
##########
core/src/main/java/org/apache/calcite/rel/rules/IntersectToDistinctRule.java:
##########
@@ -117,7 +134,7 @@ public IntersectToDistinctRule(Class<? extends Intersect> intersectClass,
for (int i = 0; i < branchCount; i++) {
filters.add(
relBuilder.greaterThan(relBuilder.field(oriFieldCount + i),
- relBuilder.literal(0)));
+ relBuilder.literal(0)));
Review Comment:
But I think the original version of the indentation modification is reasonable. There is no need to change it back. I probably didn't adjust the indentation in the original rule correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on PR #4494:
URL: https://github.com/apache/calcite/pull/4494#issuecomment-3165498860
Squashing before merging
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4494:
URL: https://github.com/apache/calcite/pull/4494#issuecomment-3163128889
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4494) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4494&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4494&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4494&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4494&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4494&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4494)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4494:
URL: https://github.com/apache/calcite/pull/4494#discussion_r2259522008
##########
core/src/main/java/org/apache/calcite/rel/rules/IntersectToDistinctRule.java:
##########
@@ -117,7 +134,7 @@ public IntersectToDistinctRule(Class<? extends Intersect> intersectClass,
for (int i = 0; i < branchCount; i++) {
filters.add(
relBuilder.greaterThan(relBuilder.field(oriFieldCount + i),
- relBuilder.literal(0)));
+ relBuilder.literal(0)));
Review Comment:
Sorry @xiedeyantu, I have pushed the change before reading your comment. I share your same feeling, but as long as autostyle is happy, I don't feel strongly about the indentation format, we can keep your original one, restored by my latest commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4494:
URL: https://github.com/apache/calcite/pull/4494#issuecomment-3165465193
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4494) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4494&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4494&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4494&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4494&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4494&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4494)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on PR #4494:
URL: https://github.com/apache/calcite/pull/4494#issuecomment-3159824988
Thanks for the review @xiedeyantu, I will wait for 24 hours before merging to allow people to either take a look or require a further extension if they need it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7125] Impossible to get a plan with partial aggregate push-down via IntersectToDistinctRule [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4494:
URL: https://github.com/apache/calcite/pull/4494#discussion_r2259497506
##########
core/src/main/java/org/apache/calcite/rel/rules/IntersectToDistinctRule.java:
##########
@@ -117,7 +134,7 @@ public IntersectToDistinctRule(Class<? extends Intersect> intersectClass,
for (int i = 0; i < branchCount; i++) {
filters.add(
relBuilder.greaterThan(relBuilder.field(oriFieldCount + i),
- relBuilder.literal(0)));
+ relBuilder.literal(0)));
Review Comment:
But I think the original version of the indentation modification is reasonable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org