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/09/16 12:17:58 UTC
[GitHub] [calcite] wojustme opened a new pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
wojustme opened a new pull request #2522:
URL: https://github.com/apache/calcite/pull/2522
Enhance `org.apache.calcite.plan.SubstitutionVisitor#unifyAggregates` to support the case, which is that aggcall of query could be expressed by target's grouping.
--
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] wojustme commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r727855771
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
Thanks a lot.
It's really my mistake.
I have updated pr.
please it again.
--
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] yanlin-Lynn commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r728593518
##########
File path: core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java
##########
@@ -1135,6 +1135,103 @@
sql(mv, query).ok();
}
+ /**
+ * Agg-call in query couldn't be generated by mv's grouping.
+ * Because this arg is neither distinct nor Optionality.IGNORED
+ */
+ @Test void testAggDistinctInMvGrouping() {
+ final String mv = ""
+ + "select \"deptno\", \"name\""
+ + "from \"emps\" group by \"deptno\", \"name\"";
+ final String query = ""
+ + "select \"deptno\", \"name\", count(distinct \"name\")"
+ + "from \"emps\" group by \"deptno\", \"name\"";
+ sql(mv, query).ok();
+ }
+
+ /**
+ * Agg-call in query couldn't be generated by mv's grouping.
+ * Because this arg is neither distinct nor Optionality.IGNORED
Review comment:
This doc does not match with the 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r727872033
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
@asolimando , the value of count(*) in query1 is always 1. But in the original query it can be any number.
--
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] yanlin-Lynn commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r727828358
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
```
--query:
select deptno, commission, count(*) as c, max(salary) as m
from emps
group by deptno, commission
--transform query to query1:
select deptno, commission, count(*) as c, m as m
from (
select deptno, commission, max(salary) as m
from emps
group by deptno, commission
) group by deptno, commission, m
```
no, you cannot do this. These two queries are not equal.
--
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] asolimando commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
asolimando commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r727838498
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
@yanlin-Lynn I think that hinting on the reason why the two are not equivalent can be of great help for the contributor
--
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] wojustme commented on pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#issuecomment-922272368
@yanlin-Lynn @hsyuan @xy2953396112
Please take a time to review this pr.
Thanks a lot.
--
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] wojustme commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r725789741
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
Hi @xiayutianwei1994 Could I give an example to prove it?
`
--mv1:
select deptno, commission, max(salary) as m
from emps
group by deptno, commission
--mv2:
select deptno, commission, m
from (
select deptno, commission, max(salary) as m
from emps
group by deptno, commission
) group by deptno, commission, m
`
We found that mv1 is equal to mv2.
-------
`
--query:
select deptno, commission, count(*) as c, max(salary) as m
from emps
group by deptno, commission
--transform query to query1:
select deptno, commission, count(*) as c, m as m
from (
select deptno, commission, max(salary) as m
from emps
group by deptno, commission
) group by deptno, commission, m
`
query1 is equal to query, because of inner grouping of (deptno, commission) is equal to out grouping of (deptno, commission, m).
query1 could be rewritten by mv2, so I think that origin query also could be rewritten by mv1.
I don't know whether my analysis is correct？ Please point out my mistake, if this analysis is wrong.
Otherwise, I will make a new pr to enhance it.
------
Also, please review this comment. @yanlin-Lynn @hsyuan :)
--
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] wojustme commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r727857533
##########
File path: core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java
##########
@@ -1051,6 +1051,107 @@
sql(mv, query).ok();
}
+ /**
+ * Agg-call in query couldn't be generated by mv's grouping.
+ * Because this arg is neither distinct nor Optionality.IGNORED
+ */
+ @Test void testAggDistinctInMvGrouping() {
+ final String mv = ""
+ + "select \"deptno\", \"name\""
+ + "from \"emps\" group by \"deptno\", \"name\"";
+ final String query = ""
+ + "select \"deptno\", \"name\", count(distinct \"name\")"
+ + "from \"emps\" group by \"deptno\", \"name\"";
+ sql(mv, query).ok();
+ }
+
+ /**
+ * Agg-call in query couldn't be generated by mv's grouping.
+ * Because this arg is neither distinct nor Optionality.IGNORED
+ */
+ @Test void testGenerateQueryAggCallByMvGroupingFail() {
+ final String mv = ""
+ + "select \"deptno\", \"salary\""
+ + "from \"emps\" group by \"deptno\", \"salary\"";
+ final String query = ""
+ + "select \"deptno\", sum(\"salary\")"
+ + "from \"emps\" group by \"deptno\"";
+ sql(mv, query).noMat();
+ }
+
+ /**
+ * Agg-call in query couldn't be generated by mv's grouping.
+ * Because args of agg-call is emtpy.
+ */
+ @Test void testGenerateQueryAggCallByMvGroupingForEmptyArg1() {
+ final String mv = ""
+ + "select \"deptno\""
+ + "from \"emps\" group by \"deptno\"";
+ final String query = ""
+ + "select \"deptno\", count(*)"
+ + "from \"emps\" group by \"deptno\"";
+ sql(mv, query).ok();
Review comment:
Ok, I have updated 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
[GitHub] [calcite] wojustme commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r728620369
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1906,18 +1907,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> targetGroupGenAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ targetGroupGenAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!targetGroupGenAggCalls.isEmpty()) {
+ return null;
Review comment:
Ok, I added some doc.
--
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] wojustme commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r716215677
##########
File path: core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java
##########
@@ -1051,6 +1051,59 @@
sql(mv, query).ok();
}
+ @Test void testAggCallArgExpressedByMvProjs1() {
+ final String mv = ""
+ + "select \"deptno\", \"salary\""
+ + "from \"emps\" group by \"deptno\", \"salary\"";
+ final String query = ""
+ + "select \"deptno\", sum(\"salary\")"
+ + "from \"emps\" group by \"deptno\"";
+ sql(mv, query).ok();
Review comment:
Yeah, You are right.
It's incorrect test case.
I have updated pr, please review it again.
Thanks a lot.
--
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] wojustme commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r725487382
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
@xiayutianwei1994
You said the opposite. It return null, when autoCreatedAggCalls isn't empty.
For example:(This case couldn't use mv to execute rewriting query's plan by mv-match)
```
--mv
select deptno, commission, max(salary) from emps group by deptno, commission
--query
select deptno, commission, count(*), max(salary) from emps group by deptno, commission
```
--
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] wojustme commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r725487382
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
@xiayutianwei1994
You said the opposite. It return null, when autoCreatedAggCalls(generated by mv's grouping) isn't empty.
There is an interesting example:
```
-- mv
select deptno, commission, max(salary) as m from emps group by deptno, commission
-- query
select deptno, commission, count(*) as c, max(salary) as m from emps group by deptno, commission
-- after mv-matching
select deptno, commission, count(*) as c, m
from mv
group by deptno, commission, m
```
max(salary) is based on two grouping of deptno, commission.
So, group's num of `select deptno, commission, m from mv group by deptno, commission, m` is equal to group's num of `select deptno, commission from mv group by deptno, commission`
But, this situation is too complicated. I want to strengthen this code gradually.
--
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] yanlin-Lynn commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r728591010
##########
File path: core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java
##########
@@ -1135,6 +1135,103 @@
sql(mv, query).ok();
}
+ /**
+ * Agg-call in query couldn't be generated by mv's grouping.
+ * Because this arg is neither distinct nor Optionality.IGNORED
Review comment:
This doc does not match with the 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r727872416
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1906,18 +1907,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> targetGroupGenAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ targetGroupGenAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!targetGroupGenAggCalls.isEmpty()) {
+ return null;
Review comment:
I think you don't need to return null here.
--
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] wojustme commented on pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#issuecomment-941017204
Hi @yanlin-Lynn @xiayutianwei1994 @hsyuan
Please review this pr, if you have a time.
If pr is ok, please ping me to rebase master.
--
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] wojustme commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r727904036
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1906,18 +1907,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> targetGroupGenAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ targetGroupGenAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!targetGroupGenAggCalls.isEmpty()) {
+ return null;
Review comment:
It's other issue, I want to support it in next pr.
For example:
```
mv: select deptno, salary, min(commission) from emps group by deptno, salary
query: select deptno, salary, max(salary), min(commission) from emps group by deptno, salary
```
In order to mv-matching, we need put `min(commission)` into aggregate's grouping.
It's too complicated
--
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] wojustme commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r725789741
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
Hi @xiayutianwei1994
I will give an example to prove it?
```
--mv1:
select deptno, commission, max(salary) as m
from emps
group by deptno, commission
--mv2:
select deptno, commission, m
from (
select deptno, commission, max(salary) as m
from emps
group by deptno, commission
) group by deptno, commission, m
```
**We found that mv1 is equal to mv2.**
---
```
--query:
select deptno, commission, count(*) as c, max(salary) as m
from emps
group by deptno, commission
--transform query to query1:
select deptno, commission, count(*) as c, m as m
from (
select deptno, commission, max(salary) as m
from emps
group by deptno, commission
) group by deptno, commission, m
```
query1 is equal to query, because of inner grouping of (deptno, commission) is equal to out grouping of (deptno, commission, m).
query1 could be rewritten by mv2, so I think that **_origin query also could be rewritten by mv1._**
---
I don't know whether my analysis is correct？ Please point out my mistake, if this analysis is wrong.
Otherwise, I will make a new pr to enhance it.
Also, please review this comment. @yanlin-Lynn @hsyuan
Thanks a lot :)
--
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] wojustme commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r727855771
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
Thanks a lot.
It's really my mistake.
I have updated pr.
please it again.
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
@yanlin-Lynn @asolimando
Thanks a lot.
It's really my mistake.
I have updated pr.
please it again.
##########
File path: core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java
##########
@@ -1051,6 +1051,107 @@
sql(mv, query).ok();
}
+ /**
+ * Agg-call in query couldn't be generated by mv's grouping.
+ * Because this arg is neither distinct nor Optionality.IGNORED
+ */
+ @Test void testAggDistinctInMvGrouping() {
+ final String mv = ""
+ + "select \"deptno\", \"name\""
+ + "from \"emps\" group by \"deptno\", \"name\"";
+ final String query = ""
+ + "select \"deptno\", \"name\", count(distinct \"name\")"
+ + "from \"emps\" group by \"deptno\", \"name\"";
+ sql(mv, query).ok();
+ }
+
+ /**
+ * Agg-call in query couldn't be generated by mv's grouping.
+ * Because this arg is neither distinct nor Optionality.IGNORED
+ */
+ @Test void testGenerateQueryAggCallByMvGroupingFail() {
+ final String mv = ""
+ + "select \"deptno\", \"salary\""
+ + "from \"emps\" group by \"deptno\", \"salary\"";
+ final String query = ""
+ + "select \"deptno\", sum(\"salary\")"
+ + "from \"emps\" group by \"deptno\"";
+ sql(mv, query).noMat();
+ }
+
+ /**
+ * Agg-call in query couldn't be generated by mv's grouping.
+ * Because args of agg-call is emtpy.
+ */
+ @Test void testGenerateQueryAggCallByMvGroupingForEmptyArg1() {
+ final String mv = ""
+ + "select \"deptno\""
+ + "from \"emps\" group by \"deptno\"";
+ final String query = ""
+ + "select \"deptno\", count(*)"
+ + "from \"emps\" group by \"deptno\"";
+ sql(mv, query).ok();
Review comment:
Ok, I have updated it.
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1994,6 +2009,45 @@ public static MutableAggregate permute(MutableAggregate aggregate,
return result;
}
+ /**
+ * Generate agg call by mv's grouping.
+ */
+ private static @Nullable AggregateCall genAggCallWithTargetGrouping(AggregateCall queryAggCall,
+ List<Integer> targetGroupByIndexes) {
+ final SqlAggFunction aggregation = queryAggCall.getAggregation();
+ final List<Integer> argList = queryAggCall.getArgList();
+ final List<Integer> newArgList = new ArrayList<>();
+ for (Integer arg : argList) {
+ final int newArgIndex = targetGroupByIndexes.indexOf(arg);
+ if (newArgIndex < 0) {
+ return null;
+ }
+ newArgList.add(newArgIndex);
+ }
+ final boolean isAllowBuild;
+ if (newArgList.size() == 0) {
+ // Size of agg-call's args, we can build a new agg-call.
Review comment:
Yes, It's my wrong.
It's updated.
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1906,18 +1907,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> targetGroupGenAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ targetGroupGenAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!targetGroupGenAggCalls.isEmpty()) {
+ return null;
Review comment:
It's other issue, I want to support it in next pr.
For example:
```
mv: select deptno, salary, min(commission) from emps group by deptno, salary
query: select deptno, salary, max(salary), min(commission) from emps group by deptno, salary
```
In order to mv-matching, we need put `min(commission)` into aggregate's grouping.
It's too complicated
--
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] yanlin-Lynn commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r716528821
##########
File path: core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java
##########
@@ -1051,6 +1051,69 @@
sql(mv, query).ok();
}
+ @Test void testAggCallArgExpressedByMvProjs1ForFail() {
Review comment:
You are fixing two mv recognition cases in one pr, 'max' and 'count(1) or count(*)' in query.
But actually they are not related, can you split this into two prs?
--
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] yanlin-Lynn commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r728590344
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1906,18 +1907,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> targetGroupGenAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ targetGroupGenAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!targetGroupGenAggCalls.isEmpty()) {
+ return null;
Review comment:
During mv recognition, the `max(salary)` in query need to create a aggregate in order to rewrite, but the `min(commission)` in query need to create a calc. So you add this code to prevent the mix cases. Am i right?
--
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] xiayutianwei1994 commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
xiayutianwei1994 commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r725448443
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
I'm a little confused about this code, why need to return null when autoCreatedAggCalls is empty.
--
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] wojustme commented on pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#issuecomment-938614549
Hi @hsyuan
Could I take your time to review this pr.
As [issue](https://issues.apache.org/jira/browse/CALCITE-4817?focusedCommentId=17426108&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17426108) discussed, this pr will fix the bug.
I'm sorry for making an imperfect pr: https://github.com/apache/calcite/pull/2556
------------------------------
If this pr is ok, I will rebase master to solve the conflicts.
--
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] yanlin-Lynn commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r727832887
##########
File path: core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java
##########
@@ -1051,6 +1051,107 @@
sql(mv, query).ok();
}
+ /**
+ * Agg-call in query couldn't be generated by mv's grouping.
+ * Because this arg is neither distinct nor Optionality.IGNORED
+ */
+ @Test void testAggDistinctInMvGrouping() {
+ final String mv = ""
+ + "select \"deptno\", \"name\""
+ + "from \"emps\" group by \"deptno\", \"name\"";
+ final String query = ""
+ + "select \"deptno\", \"name\", count(distinct \"name\")"
+ + "from \"emps\" group by \"deptno\", \"name\"";
+ sql(mv, query).ok();
+ }
+
+ /**
+ * Agg-call in query couldn't be generated by mv's grouping.
+ * Because this arg is neither distinct nor Optionality.IGNORED
+ */
+ @Test void testGenerateQueryAggCallByMvGroupingFail() {
+ final String mv = ""
+ + "select \"deptno\", \"salary\""
+ + "from \"emps\" group by \"deptno\", \"salary\"";
+ final String query = ""
+ + "select \"deptno\", sum(\"salary\")"
+ + "from \"emps\" group by \"deptno\"";
+ sql(mv, query).noMat();
+ }
+
+ /**
+ * Agg-call in query couldn't be generated by mv's grouping.
+ * Because args of agg-call is emtpy.
+ */
+ @Test void testGenerateQueryAggCallByMvGroupingForEmptyArg1() {
+ final String mv = ""
+ + "select \"deptno\""
+ + "from \"emps\" group by \"deptno\"";
+ final String query = ""
+ + "select \"deptno\", count(*)"
+ + "from \"emps\" group by \"deptno\"";
+ sql(mv, query).ok();
Review comment:
this should not match.
--
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] wojustme commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r727858317
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1994,6 +2009,45 @@ public static MutableAggregate permute(MutableAggregate aggregate,
return result;
}
+ /**
+ * Generate agg call by mv's grouping.
+ */
+ private static @Nullable AggregateCall genAggCallWithTargetGrouping(AggregateCall queryAggCall,
+ List<Integer> targetGroupByIndexes) {
+ final SqlAggFunction aggregation = queryAggCall.getAggregation();
+ final List<Integer> argList = queryAggCall.getArgList();
+ final List<Integer> newArgList = new ArrayList<>();
+ for (Integer arg : argList) {
+ final int newArgIndex = targetGroupByIndexes.indexOf(arg);
+ if (newArgIndex < 0) {
+ return null;
+ }
+ newArgList.add(newArgIndex);
+ }
+ final boolean isAllowBuild;
+ if (newArgList.size() == 0) {
+ // Size of agg-call's args, we can build a new agg-call.
Review comment:
Yes, It's my wrong.
It's updated.
--
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] yanlin-Lynn commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r727822436
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1994,6 +2009,45 @@ public static MutableAggregate permute(MutableAggregate aggregate,
return result;
}
+ /**
+ * Generate agg call by mv's grouping.
+ */
+ private static @Nullable AggregateCall genAggCallWithTargetGrouping(AggregateCall queryAggCall,
+ List<Integer> targetGroupByIndexes) {
+ final SqlAggFunction aggregation = queryAggCall.getAggregation();
+ final List<Integer> argList = queryAggCall.getArgList();
+ final List<Integer> newArgList = new ArrayList<>();
+ for (Integer arg : argList) {
+ final int newArgIndex = targetGroupByIndexes.indexOf(arg);
+ if (newArgIndex < 0) {
+ return null;
+ }
+ newArgList.add(newArgIndex);
+ }
+ final boolean isAllowBuild;
+ if (newArgList.size() == 0) {
+ // Size of agg-call's args, we can build a new agg-call.
Review comment:
newArgList.size == 0 only exists when the query agg is count(*), right?
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
```
--query:
select deptno, commission, count(*) as c, max(salary) as m
from emps
group by deptno, commission
--transform query to query1:
select deptno, commission, count(*) as c, m as m
from (
select deptno, commission, max(salary) as m
from emps
group by deptno, commission
) group by deptno, commission, m
```
no, you cannot do this. These two queries are not equal.
##########
File path: core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java
##########
@@ -1051,6 +1051,107 @@
sql(mv, query).ok();
}
+ /**
+ * Agg-call in query couldn't be generated by mv's grouping.
+ * Because this arg is neither distinct nor Optionality.IGNORED
+ */
+ @Test void testAggDistinctInMvGrouping() {
+ final String mv = ""
+ + "select \"deptno\", \"name\""
+ + "from \"emps\" group by \"deptno\", \"name\"";
+ final String query = ""
+ + "select \"deptno\", \"name\", count(distinct \"name\")"
+ + "from \"emps\" group by \"deptno\", \"name\"";
+ sql(mv, query).ok();
+ }
+
+ /**
+ * Agg-call in query couldn't be generated by mv's grouping.
+ * Because this arg is neither distinct nor Optionality.IGNORED
+ */
+ @Test void testGenerateQueryAggCallByMvGroupingFail() {
+ final String mv = ""
+ + "select \"deptno\", \"salary\""
+ + "from \"emps\" group by \"deptno\", \"salary\"";
+ final String query = ""
+ + "select \"deptno\", sum(\"salary\")"
+ + "from \"emps\" group by \"deptno\"";
+ sql(mv, query).noMat();
+ }
+
+ /**
+ * Agg-call in query couldn't be generated by mv's grouping.
+ * Because args of agg-call is emtpy.
+ */
+ @Test void testGenerateQueryAggCallByMvGroupingForEmptyArg1() {
+ final String mv = ""
+ + "select \"deptno\""
+ + "from \"emps\" group by \"deptno\"";
+ final String query = ""
+ + "select \"deptno\", count(*)"
+ + "from \"emps\" group by \"deptno\"";
+ sql(mv, query).ok();
Review comment:
this should not match.
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1994,6 +2009,45 @@ public static MutableAggregate permute(MutableAggregate aggregate,
return result;
}
+ /**
+ * Generate agg call by mv's grouping.
+ */
+ private static @Nullable AggregateCall genAggCallWithTargetGrouping(AggregateCall queryAggCall,
+ List<Integer> targetGroupByIndexes) {
+ final SqlAggFunction aggregation = queryAggCall.getAggregation();
+ final List<Integer> argList = queryAggCall.getArgList();
+ final List<Integer> newArgList = new ArrayList<>();
+ for (Integer arg : argList) {
+ final int newArgIndex = targetGroupByIndexes.indexOf(arg);
+ if (newArgIndex < 0) {
+ return null;
+ }
+ newArgList.add(newArgIndex);
+ }
+ final boolean isAllowBuild;
+ if (newArgList.size() == 0) {
+ // Size of agg-call's args, we can build a new agg-call.
Review comment:
What's the mean of the note `Size of agg-call's args, we can build a new agg-call.`
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
@asolimando , the value of count(*) in query1 is always 1. But in the original query it can be any number.
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1906,18 +1907,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> targetGroupGenAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ targetGroupGenAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!targetGroupGenAggCalls.isEmpty()) {
+ return null;
Review comment:
I think you don't need to return null here.
--
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] wojustme commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r725789741
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
Hi @xiayutianwei1994
I will give an example to prove it?
```
--mv1:
select deptno, commission, max(salary) as m
from emps
group by deptno, commission
--mv2:
select deptno, commission, m
from (
select deptno, commission, max(salary) as m
from emps
group by deptno, commission
) group by deptno, commission, m
```
**We found that mv1 is equal to mv2.**
In compute's view, the result of mv1 is equal to mv2's, because of the same duplicate grouping info.
---
```
--query:
select deptno, commission, count(*) as c, max(salary) as m
from emps
group by deptno, commission
--transform query to query1:
select deptno, commission, count(*) as c, m as m
from (
select deptno, commission, max(salary) as m
from emps
group by deptno, commission
) group by deptno, commission, m
```
query1 is equal to query, because of inner grouping of (deptno, commission) is equal to out grouping of (deptno, commission, m).
query1 could be rewritten by mv2, so I think that **_origin query also could be rewritten by mv1._**
---
I don't know whether my analysis is correct？ Please point out my mistake, if this analysis is wrong.
Otherwise, I will make a new pr to enhance it.
Also, please review this comment. @yanlin-Lynn @hsyuan
Thanks a lot :)
--
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] yanlin-Lynn commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r716177327
##########
File path: core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java
##########
@@ -1051,6 +1051,59 @@
sql(mv, query).ok();
}
+ @Test void testAggCallArgExpressedByMvProjs1() {
+ final String mv = ""
+ + "select \"deptno\", \"salary\""
+ + "from \"emps\" group by \"deptno\", \"salary\"";
+ final String query = ""
+ + "select \"deptno\", sum(\"salary\")"
+ + "from \"emps\" group by \"deptno\"";
+ sql(mv, query).ok();
Review comment:
This is not correct, should not match in this 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r728656587
##########
File path: core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java
##########
@@ -1135,6 +1135,85 @@
sql(mv, query).ok();
}
+ /**
+ * It's match, distinct agg-call could be expressed by mv's grouping.
+ */
+ @Test void testAggDistinctInMvGrouping() {
+ final String mv = ""
+ + "select \"deptno\", \"name\""
+ + "from \"emps\" group by \"deptno\", \"name\"";
+ final String query = ""
+ + "select \"deptno\", \"name\", count(distinct \"name\")"
+ + "from \"emps\" group by \"deptno\", \"name\"";
+ sql(mv, query).ok();
+ }
+
+ /**
+ * It's match, `Optionality.IGNORED` agg-call could be expressed by mv's grouping.
+ */
+ @Test void testAggOptionalityInMvGrouping() {
+ final String mv = ""
+ + "select \"deptno\", \"salary\""
+ + "from \"emps\" group by \"deptno\", \"salary\"";
+ final String query = ""
+ + "select \"deptno\", \"salary\", max(\"salary\")"
+ + "from \"emps\" group by \"deptno\", \"salary\"";
+ sql(mv, query).ok();
+ }
+
+ /**
+ * It's match, normal agg-call could be expressed by mv's grouping.
Review comment:
This doc is not match with the 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r727822436
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1994,6 +2009,45 @@ public static MutableAggregate permute(MutableAggregate aggregate,
return result;
}
+ /**
+ * Generate agg call by mv's grouping.
+ */
+ private static @Nullable AggregateCall genAggCallWithTargetGrouping(AggregateCall queryAggCall,
+ List<Integer> targetGroupByIndexes) {
+ final SqlAggFunction aggregation = queryAggCall.getAggregation();
+ final List<Integer> argList = queryAggCall.getArgList();
+ final List<Integer> newArgList = new ArrayList<>();
+ for (Integer arg : argList) {
+ final int newArgIndex = targetGroupByIndexes.indexOf(arg);
+ if (newArgIndex < 0) {
+ return null;
+ }
+ newArgList.add(newArgIndex);
+ }
+ final boolean isAllowBuild;
+ if (newArgList.size() == 0) {
+ // Size of agg-call's args, we can build a new agg-call.
Review comment:
newArgList.size == 0 only exists when the query agg is count(*), right?
--
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] wojustme commented on pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#issuecomment-939303965
@hsyuan
I am sorry to disturb you again.
Please review this pr.
Thanks a lot.
--
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] wojustme edited a comment on pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme edited a comment on pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#issuecomment-938614549
Hi @hsyuan
Could I take your time to review this pr.
As [issue](https://issues.apache.org/jira/browse/CALCITE-4817?focusedCommentId=17426108&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17426108) discussed, this pr will fix the bug.
I'm sorry for making an imperfect pr: https://github.com/apache/calcite/pull/2556
------------------------------
If this pr is ok, please tell me.
I will rebase master to solve the conflicts.
--
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] yanlin-Lynn commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r728597810
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1906,18 +1907,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> targetGroupGenAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ targetGroupGenAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!targetGroupGenAggCalls.isEmpty()) {
+ return null;
Review comment:
Can you doc the reason here
--
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] asolimando commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
asolimando commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r727838498
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
@yanlin-Lynn I think that hinting on the reason why the two are not equivalent can be of great help for the contributor
--
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] yanlin-Lynn commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r727867615
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1994,6 +2009,45 @@ public static MutableAggregate permute(MutableAggregate aggregate,
return result;
}
+ /**
+ * Generate agg call by mv's grouping.
+ */
+ private static @Nullable AggregateCall genAggCallWithTargetGrouping(AggregateCall queryAggCall,
+ List<Integer> targetGroupByIndexes) {
+ final SqlAggFunction aggregation = queryAggCall.getAggregation();
+ final List<Integer> argList = queryAggCall.getArgList();
+ final List<Integer> newArgList = new ArrayList<>();
+ for (Integer arg : argList) {
+ final int newArgIndex = targetGroupByIndexes.indexOf(arg);
+ if (newArgIndex < 0) {
+ return null;
+ }
+ newArgList.add(newArgIndex);
+ }
+ final boolean isAllowBuild;
+ if (newArgList.size() == 0) {
+ // Size of agg-call's args, we can build a new agg-call.
Review comment:
What's the mean of the note `Size of agg-call's args, we can build a new agg-call.`
--
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] wojustme commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r716579325
##########
File path: core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java
##########
@@ -1051,6 +1051,69 @@
sql(mv, query).ok();
}
+ @Test void testAggCallArgExpressedByMvProjs1ForFail() {
Review comment:
Ok, I'll split it into two pr.
Thanks for your review.
--
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] wojustme commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r725487382
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
@xiayutianwei1994
You said the opposite. It return null, when autoCreatedAggCalls isn't empty.
There is an interesting example:
```
-- mv
select deptno, commission, max(salary) as m from emps group by deptno, commission
-- query
select deptno, commission, count(*) as c, max(salary) as m from emps group by deptno, commission
-- after mv-matching
select deptno, commission, count(*) as c, m
from mv
group by deptno, commission, m
```
max(salary) is based on two grouping of deptno, commission.
So, group's num of `select deptno, commission, m from mv group by deptno, commission, m` is equal to group's num of `select deptno, commission from mv group by deptno, commission`
But, this situation is too complicated. I want to strengthen this code gradually.
--
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] wojustme commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r728620898
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1994,6 +2009,45 @@ public static MutableAggregate permute(MutableAggregate aggregate,
return result;
}
+ /**
+ * Generate agg call by mv's grouping.
+ */
+ private static @Nullable AggregateCall genAggCallWithTargetGrouping(AggregateCall queryAggCall,
+ List<Integer> targetGroupByIndexes) {
+ final SqlAggFunction aggregation = queryAggCall.getAggregation();
+ final List<Integer> argList = queryAggCall.getArgList();
+ final List<Integer> newArgList = new ArrayList<>();
+ for (Integer arg : argList) {
+ final int newArgIndex = targetGroupByIndexes.indexOf(arg);
+ if (newArgIndex < 0) {
+ return null;
+ }
+ newArgList.add(newArgIndex);
+ }
+ final boolean isAllowBuild;
+ if (newArgList.size() == 0) {
+ // Size of agg-call's args, we can build a new agg-call.
Review comment:
I updated the doc for this code.
--
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] wojustme commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
wojustme commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r727855771
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
@yanlin-Lynn @asolimando
Thanks a lot.
It's really my mistake.
I have updated pr.
please it again.
--
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] xiayutianwei1994 commented on a change in pull request #2522: [CALCITE-4735] Enhance Aggregate-Materialization, when aggcall of query could be expressed by target's grouping (xurenhe)
Posted by GitBox <gi...@apache.org>.
xiayutianwei1994 commented on a change in pull request #2522:
URL: https://github.com/apache/calcite/pull/2522#discussion_r725756200
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1914,18 +1915,36 @@ public static MutableAggregate permute(MutableAggregate aggregate,
int i = targetGroupByIndexList.indexOf(inputIndex);
projects.add(i);
}
+ final List<AggregateCall> autoCreatedAggCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
int i = target.aggCalls.indexOf(aggregateCall);
if (i < 0) {
- return null;
+ final AggregateCall newAggCall = genAggCallWithTargetGrouping(
+ aggregateCall, targetGroupByIndexList);
+ if (newAggCall == null) {
+ return null;
+ } else {
+ // Here, we create a new `MutableAggregate` to return.
+ // So, we record this new agg-call.
+ autoCreatedAggCalls.add(newAggCall);
+ }
+ } else {
+ if (!autoCreatedAggCalls.isEmpty()) {
+ return null;
+ }
Review comment:
I understand why need to return null, thanks.
But this example, I don't think the query can be rewritten.
After mv-matching, count(*) will be all number 1? I don't know if I am right.
--
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