You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/06/14 07:50:41 UTC
[GitHub] [calcite] devozerov opened a new pull request #2437: [CALCITE-4616] AggregateUnionTransposeRule causes row type mismatch when some inputs have unique grouping key (Vladimir Ozerov)
devozerov opened a new pull request #2437:
URL: https://github.com/apache/calcite/pull/2437
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] devozerov commented on a change in pull request #2437: [CALCITE-4616] AggregateUnionTransposeRule causes row type mismatch when some inputs have unique grouping key (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2437:
URL: https://github.com/apache/calcite/pull/2437#discussion_r673649384
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/AggregateUnionTransposeRule.java
##########
@@ -125,30 +125,34 @@ public AggregateUnionTransposeRule(Class<? extends Aggregate> aggregateClass,
return;
}
- // create corresponding aggregates on top of each union child
- final RelBuilder relBuilder = call.builder();
- int transformCount = 0;
+ boolean hasNonUniqueInputs = false;
final RelMetadataQuery mq = call.getMetadataQuery();
for (RelNode input : union.getInputs()) {
boolean alreadyUnique =
RelMdUtil.areColumnsDefinitelyUnique(mq, input,
aggRel.getGroupSet());
- relBuilder.push(input);
if (!alreadyUnique) {
- ++transformCount;
- relBuilder.aggregate(relBuilder.groupKey(aggRel.getGroupSet()),
- aggRel.getAggCallList());
+ hasNonUniqueInputs = true;
+ break;
}
}
- if (transformCount == 0) {
+ if (!hasNonUniqueInputs) {
Review comment:
Makes sense, I've renamed the variable.
--
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
[GitHub] [calcite] devozerov commented on a change in pull request #2437: [CALCITE-4616] AggregateUnionTransposeRule causes row type mismatch when some inputs have unique grouping key (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2437:
URL: https://github.com/apache/calcite/pull/2437#discussion_r652092993
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/AggregateUnionTransposeRule.java
##########
@@ -125,30 +125,34 @@ public AggregateUnionTransposeRule(Class<? extends Aggregate> aggregateClass,
return;
}
- // create corresponding aggregates on top of each union child
- final RelBuilder relBuilder = call.builder();
- int transformCount = 0;
+ boolean hasNonUniqueInputs = false;
final RelMetadataQuery mq = call.getMetadataQuery();
for (RelNode input : union.getInputs()) {
boolean alreadyUnique =
RelMdUtil.areColumnsDefinitelyUnique(mq, input,
aggRel.getGroupSet());
- relBuilder.push(input);
if (!alreadyUnique) {
- ++transformCount;
- relBuilder.aggregate(relBuilder.groupKey(aggRel.getGroupSet()),
- aggRel.getAggCallList());
+ hasNonUniqueInputs = true;
+ break;
}
}
- if (transformCount == 0) {
+ if (!hasNonUniqueInputs) {
Review comment:
This variable tracks whether there is at least one input where the group key is not unique. If there are no such inputs (i.e. all inputs already unique on the grouping key), then we return, because the pushdown doesn't make sense. Otherwise - if there is at least one non-unique input - we do the pushdown to all inputs.
Before the fix, we may do a push-down only to some inputs, which might lead to row type mismatch.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] asfgit closed pull request #2437: [CALCITE-4616] AggregateUnionTransposeRule causes row type mismatch when some inputs have unique grouping key (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #2437:
URL: https://github.com/apache/calcite/pull/2437
--
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
[GitHub] [calcite] amaliujia commented on a change in pull request #2437: [CALCITE-4616] AggregateUnionTransposeRule causes row type mismatch when some inputs have unique grouping key (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2437:
URL: https://github.com/apache/calcite/pull/2437#discussion_r652155336
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/AggregateUnionTransposeRule.java
##########
@@ -125,30 +125,34 @@ public AggregateUnionTransposeRule(Class<? extends Aggregate> aggregateClass,
return;
}
- // create corresponding aggregates on top of each union child
- final RelBuilder relBuilder = call.builder();
- int transformCount = 0;
+ boolean hasNonUniqueInputs = false;
final RelMetadataQuery mq = call.getMetadataQuery();
for (RelNode input : union.getInputs()) {
boolean alreadyUnique =
RelMdUtil.areColumnsDefinitelyUnique(mq, input,
aggRel.getGroupSet());
- relBuilder.push(input);
if (!alreadyUnique) {
- ++transformCount;
- relBuilder.aggregate(relBuilder.groupKey(aggRel.getGroupSet()),
- aggRel.getAggCallList());
+ hasNonUniqueInputs = true;
+ break;
}
}
- if (transformCount == 0) {
+ if (!hasNonUniqueInputs) {
Review comment:
Ack. Thanks for the clarification.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #2437: [CALCITE-4616] AggregateUnionTransposeRule causes row type mismatch when some inputs have unique grouping key (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2437:
URL: https://github.com/apache/calcite/pull/2437#discussion_r652083038
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/AggregateUnionTransposeRule.java
##########
@@ -125,30 +125,34 @@ public AggregateUnionTransposeRule(Class<? extends Aggregate> aggregateClass,
return;
}
- // create corresponding aggregates on top of each union child
- final RelBuilder relBuilder = call.builder();
- int transformCount = 0;
+ boolean hasNonUniqueInputs = false;
final RelMetadataQuery mq = call.getMetadataQuery();
for (RelNode input : union.getInputs()) {
boolean alreadyUnique =
RelMdUtil.areColumnsDefinitelyUnique(mq, input,
aggRel.getGroupSet());
- relBuilder.push(input);
if (!alreadyUnique) {
- ++transformCount;
- relBuilder.aggregate(relBuilder.groupKey(aggRel.getGroupSet()),
- aggRel.getAggCallList());
+ hasNonUniqueInputs = true;
+ break;
}
}
- if (transformCount == 0) {
+ if (!hasNonUniqueInputs) {
Review comment:
Using `hasUniqueKeyInAllInputs` might be better for reading (I was confused on ! + `Non` combination). Basically this says all inputs must have unique key on the same column otherwise return.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #2437: [CALCITE-4616] AggregateUnionTransposeRule causes row type mismatch when some inputs have unique grouping key (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2437:
URL: https://github.com/apache/calcite/pull/2437#discussion_r652083906
##########
File path: core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml
##########
@@ -1028,6 +1028,39 @@ LogicalProject(MGR=[$0], SUM_SAL=[$2])
LogicalAggregate(group=[{0, 1}], SUM_SAL=[SUM($2)])
LogicalProject(MGR=[$3], DEPTNO=[$7], SAL=[$5])
LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testAggregateUnionTransposeWithOneUniqueInput">
+ <Resource name="sql">
+ <![CDATA[select deptno, SUM(t) from (
+select deptno, 1 as t from sales.emp e1
+union all
+select distinct deptno, 2 as t from sales.emp e2)
+group by deptno]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalAggregate(group=[{0}], EXPR$1=[SUM($1)])
+ LogicalUnion(all=[true])
+ LogicalProject(DEPTNO=[$7], T=[1])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+ LogicalAggregate(group=[{0, 1}])
+ LogicalProject(DEPTNO=[$7], T=[2])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+LogicalAggregate(group=[{0}], EXPR$1=[SUM($1)])
Review comment:
Wait I am still confused now: if we say that all inputs have unique keys thus we push down agg, then why we still need the top agg?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] devozerov commented on a change in pull request #2437: [CALCITE-4616] AggregateUnionTransposeRule causes row type mismatch when some inputs have unique grouping key (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2437:
URL: https://github.com/apache/calcite/pull/2437#discussion_r673655623
##########
File path: core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml
##########
@@ -1028,6 +1028,39 @@ LogicalProject(MGR=[$0], SUM_SAL=[$2])
LogicalAggregate(group=[{0, 1}], SUM_SAL=[SUM($2)])
LogicalProject(MGR=[$3], DEPTNO=[$7], SAL=[$5])
LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testAggregateUnionTransposeWithOneUniqueInput">
+ <Resource name="sql">
+ <![CDATA[select deptno, SUM(t) from (
+select deptno, 1 as t from sales.emp e1
+union all
+select distinct deptno, 2 as t from sales.emp e2)
+group by deptno]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalAggregate(group=[{0}], EXPR$1=[SUM($1)])
+ LogicalUnion(all=[true])
+ LogicalProject(DEPTNO=[$7], T=[1])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+ LogicalAggregate(group=[{0, 1}])
+ LogicalProject(DEPTNO=[$7], T=[2])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+LogicalAggregate(group=[{0}], EXPR$1=[SUM($1)])
Review comment:
I added another test:
1. `testAggregateUnionTransposeWithOneInputUnique` ensures that `Aggregate` is pushed down to all inputs because at least one input is not unique.
2. `testAggregateUnionTransposeWithAllInputsUnique` ensures that `Aggregate` is not pushed because all inputs are already unique (the "return" branch).
--
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
[GitHub] [calcite] zinking commented on pull request #2437: [CALCITE-4616] AggregateUnionTransposeRule causes row type mismatch when some inputs have unique grouping key (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
zinking commented on pull request #2437:
URL: https://github.com/apache/calcite/pull/2437#issuecomment-861411898
LGTM, pretty straightfroward.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #2437: [CALCITE-4616] AggregateUnionTransposeRule causes row type mismatch when some inputs have unique grouping key (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2437:
URL: https://github.com/apache/calcite/pull/2437#discussion_r652155336
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/AggregateUnionTransposeRule.java
##########
@@ -125,30 +125,34 @@ public AggregateUnionTransposeRule(Class<? extends Aggregate> aggregateClass,
return;
}
- // create corresponding aggregates on top of each union child
- final RelBuilder relBuilder = call.builder();
- int transformCount = 0;
+ boolean hasNonUniqueInputs = false;
final RelMetadataQuery mq = call.getMetadataQuery();
for (RelNode input : union.getInputs()) {
boolean alreadyUnique =
RelMdUtil.areColumnsDefinitelyUnique(mq, input,
aggRel.getGroupSet());
- relBuilder.push(input);
if (!alreadyUnique) {
- ++transformCount;
- relBuilder.aggregate(relBuilder.groupKey(aggRel.getGroupSet()),
- aggRel.getAggCallList());
+ hasNonUniqueInputs = true;
+ break;
}
}
- if (transformCount == 0) {
+ if (!hasNonUniqueInputs) {
Review comment:
Ack. Thanks for the clarification.
However, I am thinking at least rename this to `UniqueKeysInAllInputs` or something like this?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #2437: [CALCITE-4616] AggregateUnionTransposeRule causes row type mismatch when some inputs have unique grouping key (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2437:
URL: https://github.com/apache/calcite/pull/2437#discussion_r652154431
##########
File path: core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml
##########
@@ -1028,6 +1028,39 @@ LogicalProject(MGR=[$0], SUM_SAL=[$2])
LogicalAggregate(group=[{0, 1}], SUM_SAL=[SUM($2)])
LogicalProject(MGR=[$3], DEPTNO=[$7], SAL=[$5])
LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testAggregateUnionTransposeWithOneUniqueInput">
+ <Resource name="sql">
+ <![CDATA[select deptno, SUM(t) from (
+select deptno, 1 as t from sales.emp e1
+union all
+select distinct deptno, 2 as t from sales.emp e2)
+group by deptno]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalAggregate(group=[{0}], EXPR$1=[SUM($1)])
+ LogicalUnion(all=[true])
+ LogicalProject(DEPTNO=[$7], T=[1])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+ LogicalAggregate(group=[{0, 1}])
+ LogicalProject(DEPTNO=[$7], T=[2])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+LogicalAggregate(group=[{0}], EXPR$1=[SUM($1)])
Review comment:
Ok, then the pushdown make senses since it does a pre-agg before UNION and then post-agg after UNION, that gives correct result.
And if all inputs are already unique, then there is no need to push down agg. In this case, are you aware if there is a test existing to verify the return case? (I tried to search testAggregateUnionTransposeWithAllUniqueInputs or just testAggregateUnionTranspose but only found your new test case).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] devozerov commented on a change in pull request #2437: [CALCITE-4616] AggregateUnionTransposeRule causes row type mismatch when some inputs have unique grouping key (Vladimir Ozerov)
Posted by GitBox <gi...@apache.org>.
devozerov commented on a change in pull request #2437:
URL: https://github.com/apache/calcite/pull/2437#discussion_r652093414
##########
File path: core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml
##########
@@ -1028,6 +1028,39 @@ LogicalProject(MGR=[$0], SUM_SAL=[$2])
LogicalAggregate(group=[{0, 1}], SUM_SAL=[SUM($2)])
LogicalProject(MGR=[$3], DEPTNO=[$7], SAL=[$5])
LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testAggregateUnionTransposeWithOneUniqueInput">
+ <Resource name="sql">
+ <![CDATA[select deptno, SUM(t) from (
+select deptno, 1 as t from sales.emp e1
+union all
+select distinct deptno, 2 as t from sales.emp e2)
+group by deptno]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalAggregate(group=[{0}], EXPR$1=[SUM($1)])
+ LogicalUnion(all=[true])
+ LogicalProject(DEPTNO=[$7], T=[1])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+ LogicalAggregate(group=[{0, 1}])
+ LogicalProject(DEPTNO=[$7], T=[2])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+LogicalAggregate(group=[{0}], EXPR$1=[SUM($1)])
Review comment:
We do the opposite - return if all inputs are already unique. Otherwise - do a pushdown. Please see the previous comment for more details.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org