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 2020/11/03 14:02:53 UTC
[GitHub] [calcite] xy2953396112 opened a new pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
xy2953396112 opened a new pull request #2241:
URL: https://github.com/apache/calcite/pull/2241
…nct aggregate on target GROUP BY column (xzh)
https://issues.apache.org/jira/browse/CALCITE-4374
----------------------------------------------------------------
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] xy2953396112 commented on a change in pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2241:
URL: https://github.com/apache/calcite/pull/2241#discussion_r518029726
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1930,7 +1930,20 @@ public static MutableRel unifyAggregates(MutableAggregate query,
}
final List<AggregateCall> aggregateCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
- if (aggregateCall.isDistinct()) {
+ if (aggregateCall.isDistinct() && aggregateCall.getArgList().size() == 1) {
+ int aggIndex = aggregateCall.getArgList().get(0);
+ if (target.groupSet.asSet().contains(aggIndex)) {
+ int newIndex = target.groupSet.asList().indexOf(aggIndex);
Review comment:
ok,Thanks.
----------------------------------------------------------------
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] xy2953396112 commented on a change in pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2241:
URL: https://github.com/apache/calcite/pull/2241#discussion_r518558396
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1930,7 +1929,20 @@ public static MutableRel unifyAggregates(MutableAggregate query,
}
final List<AggregateCall> aggregateCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
- if (aggregateCall.isDistinct()) {
+ if (aggregateCall.isDistinct() && aggregateCall.getArgList().size() == 1) {
+ int aggIndex = aggregateCall.getArgList().get(0);
+ if (targetGroupByIndexList.contains(aggIndex)) {
+ int newIndex = targetGroupByIndexList.indexOf(aggIndex);
+ aggregateCalls.add(
Review comment:
ok, Thanks.
----------------------------------------------------------------
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] chunweilei commented on a change in pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2241:
URL: https://github.com/apache/calcite/pull/2241#discussion_r517773759
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1930,7 +1930,20 @@ public static MutableRel unifyAggregates(MutableAggregate query,
}
final List<AggregateCall> aggregateCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
- if (aggregateCall.isDistinct()) {
+ if (aggregateCall.isDistinct() && aggregateCall.getArgList().size() == 1) {
+ int aggIndex = aggregateCall.getArgList().get(0);
+ if (target.groupSet.asSet().contains(aggIndex)) {
+ int newIndex = target.groupSet.asList().indexOf(aggIndex);
+ aggregateCalls.add(
+ AggregateCall.create(aggregateCall.getAggregation(),
Review comment:
`target.groupSet.asList()` can be replaced by `targetGroupByIndexList`?
[1]https://github.com/apache/calcite/pull/2241/files#diff-61f815e420e8c1c99c7f502f8105d6d2f91724a3bd0cb5ff4300a23ac96f527cR1898
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1930,7 +1930,20 @@ public static MutableRel unifyAggregates(MutableAggregate query,
}
final List<AggregateCall> aggregateCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
- if (aggregateCall.isDistinct()) {
+ if (aggregateCall.isDistinct() && aggregateCall.getArgList().size() == 1) {
+ int aggIndex = aggregateCall.getArgList().get(0);
+ if (target.groupSet.asSet().contains(aggIndex)) {
+ int newIndex = target.groupSet.asList().indexOf(aggIndex);
Review comment:
We'd better use a variable to represent `target.groupSet.asSet()` in case we have to transform it again and 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2241:
URL: https://github.com/apache/calcite/pull/2241#discussion_r518494152
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1930,7 +1929,20 @@ public static MutableRel unifyAggregates(MutableAggregate query,
}
final List<AggregateCall> aggregateCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
- if (aggregateCall.isDistinct()) {
+ if (aggregateCall.isDistinct() && aggregateCall.getArgList().size() == 1) {
+ int aggIndex = aggregateCall.getArgList().get(0);
+ if (targetGroupByIndexList.contains(aggIndex)) {
+ int newIndex = targetGroupByIndexList.indexOf(aggIndex);
+ aggregateCalls.add(
Review comment:
if (targetGroupByIndexList.contains(aggIndex)) is not necessary.
You
can do like this:
```
final int newIndex = targetGroupByIndexList.indexOf(aggIndex);
if (newIndex >=0 ) {
......
}
```
----------------------------------------------------------------
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] yanlin-Lynn merged pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn merged pull request #2241:
URL: https://github.com/apache/calcite/pull/2241
----------------------------------------------------------------
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] xy2953396112 commented on a change in pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2241:
URL: https://github.com/apache/calcite/pull/2241#discussion_r516736309
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1931,6 +1931,18 @@ public static MutableRel unifyAggregates(MutableAggregate query,
final List<AggregateCall> aggregateCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
if (aggregateCall.isDistinct()) {
+ int aggIndex = aggregateCall.getArgList().get(0);
Review comment:
Thanks, update the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2241:
URL: https://github.com/apache/calcite/pull/2241#discussion_r518494152
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1930,7 +1929,20 @@ public static MutableRel unifyAggregates(MutableAggregate query,
}
final List<AggregateCall> aggregateCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
- if (aggregateCall.isDistinct()) {
+ if (aggregateCall.isDistinct() && aggregateCall.getArgList().size() == 1) {
+ int aggIndex = aggregateCall.getArgList().get(0);
+ if (targetGroupByIndexList.contains(aggIndex)) {
+ int newIndex = targetGroupByIndexList.indexOf(aggIndex);
+ aggregateCalls.add(
Review comment:
if (targetGroupByIndexList.contains(aggIndex)) is not necessary.
You can do like this:
```
final int newIndex = targetGroupByIndexList.indexOf(aggIndex);
if (newIndex >=0 ) {
......
}
```
----------------------------------------------------------------
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] xy2953396112 commented on a change in pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2241:
URL: https://github.com/apache/calcite/pull/2241#discussion_r517118764
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1931,6 +1931,22 @@ public static MutableRel unifyAggregates(MutableAggregate query,
final List<AggregateCall> aggregateCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
if (aggregateCall.isDistinct()) {
+ if (aggregateCall.getArgList().size() == 1) {
+ int aggIndex = aggregateCall.getArgList().get(0);
+ if (target.groupSet.asSet().contains(aggIndex)) {
Review comment:
The aggregate function containing the distinct keyword has only one parameter. Check the parameters 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2241:
URL: https://github.com/apache/calcite/pull/2241#discussion_r518029460
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1931,6 +1931,22 @@ public static MutableRel unifyAggregates(MutableAggregate query,
final List<AggregateCall> aggregateCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
if (aggregateCall.isDistinct()) {
+ if (aggregateCall.getArgList().size() == 1) {
+ int aggIndex = aggregateCall.getArgList().get(0);
+ if (target.groupSet.asSet().contains(aggIndex)) {
Review comment:
Thanks,update the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on pull request #2241:
URL: https://github.com/apache/calcite/pull/2241#issuecomment-724403418
+1
----------------------------------------------------------------
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] chunweilei commented on a change in pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2241:
URL: https://github.com/apache/calcite/pull/2241#discussion_r517086483
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1931,6 +1931,22 @@ public static MutableRel unifyAggregates(MutableAggregate query,
final List<AggregateCall> aggregateCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
if (aggregateCall.isDistinct()) {
+ if (aggregateCall.getArgList().size() == 1) {
+ int aggIndex = aggregateCall.getArgList().get(0);
+ if (target.groupSet.asSet().contains(aggIndex)) {
Review comment:
Could you please clarify why the size of the arguments has to be 1?
----------------------------------------------------------------
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] yanlin-Lynn commented on a change in pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2241:
URL: https://github.com/apache/calcite/pull/2241#discussion_r516694228
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1931,6 +1931,18 @@ public static MutableRel unifyAggregates(MutableAggregate query,
final List<AggregateCall> aggregateCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
if (aggregateCall.isDistinct()) {
+ int aggIndex = aggregateCall.getArgList().get(0);
Review comment:
What about other args ?
----------------------------------------------------------------
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] chunweilei commented on a change in pull request #2241: [CALCITE-4374] Support materialized view recognition when query disti…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2241:
URL: https://github.com/apache/calcite/pull/2241#discussion_r518494152
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1930,7 +1929,20 @@ public static MutableRel unifyAggregates(MutableAggregate query,
}
final List<AggregateCall> aggregateCalls = new ArrayList<>();
for (AggregateCall aggregateCall : query.aggCalls) {
- if (aggregateCall.isDistinct()) {
+ if (aggregateCall.isDistinct() && aggregateCall.getArgList().size() == 1) {
+ int aggIndex = aggregateCall.getArgList().get(0);
+ if (targetGroupByIndexList.contains(aggIndex)) {
+ int newIndex = targetGroupByIndexList.indexOf(aggIndex);
+ aggregateCalls.add(
Review comment:
`if (targetGroupByIndexList.contains(aggIndex))` is not necessary.
You can do like this:
```
final int newIndex = targetGroupByIndexList.indexOf(aggIndex);
if (newIndex >=0 ) {
......
}
```
----------------------------------------------------------------
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