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/03/19 23:34:35 UTC
[GitHub] [calcite] hsyuan opened a new pull request #1863: [CALCITE-3865]
RelCollationTraitDef.canConvert should always return true
hsyuan opened a new pull request #1863: [CALCITE-3865] RelCollationTraitDef.canConvert should always return true
URL: https://github.com/apache/calcite/pull/1863
[CALCITE-1148](https://github.com/apache/calcite/pull/210) introduced changes to RelCollationTraitDef to fix RelTrait
conversion bug, but it is just hiding the underlying issue and adding redundant
check to planner.
The root cause is that logical operators, especially LogicalSort can have
traits, which is a bad design decision, and AggregateReduceFunctionsRule and
RelBuilder fail to adjust the column mapping in RelTraitSet. The newly created
LogicalProject has collation on column 5 (it just copies its input's
RelTraitSet), but it only has 2 columns.
The ideal way is to add {{ apply(Mapping) }} method to RelTrait interface and
RelTraitSet, so that we have unified way to apply column mappings. But it will
be a breaking change, I am reluctant to do it now.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1863:
[CALCITE-3865] RelCollationTraitDef.canConvert should always return true
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1863: [CALCITE-3865] RelCollationTraitDef.canConvert should always return true
URL: https://github.com/apache/calcite/pull/1863#discussion_r395441829
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -1755,7 +1755,7 @@ public RelBuilder aggregate(GroupKey groupKey, Iterable<AggCall> aggCalls) {
newProjects.add(project.getProjects().get(i));
builder.add(project.getRowType().getFieldList().get(i));
}
- r = project.copy(r.getTraitSet(), project.getInput(), newProjects,
+ r = project.copy(cluster.traitSet(), project.getInput(), newProjects,
builder.build());
Review comment:
Can we try to make the trait set correct in current 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
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1863:
[CALCITE-3865] RelCollationTraitDef.canConvert should always return true
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1863: [CALCITE-3865] RelCollationTraitDef.canConvert should always return true
URL: https://github.com/apache/calcite/pull/1863#discussion_r395485422
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -1755,7 +1755,7 @@ public RelBuilder aggregate(GroupKey groupKey, Iterable<AggCall> aggCalls) {
newProjects.add(project.getProjects().get(i));
builder.add(project.getRowType().getFieldList().get(i));
}
- r = project.copy(r.getTraitSet(), project.getInput(), newProjects,
+ r = project.copy(cluster.traitSet(), project.getInput(), newProjects,
builder.build());
Review comment:
We should not have LogicalSort. It also incorporates the functionality of LIMIT, another wrong design.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan closed pull request #1863: [CALCITE-3865]
RelCollationTraitDef.canConvert should always return true
Posted by GitBox <gi...@apache.org>.
hsyuan closed pull request #1863: [CALCITE-3865] RelCollationTraitDef.canConvert should always return true
URL: https://github.com/apache/calcite/pull/1863
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1863:
[CALCITE-3865] RelCollationTraitDef.canConvert should always return true
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1863: [CALCITE-3865] RelCollationTraitDef.canConvert should always return true
URL: https://github.com/apache/calcite/pull/1863#discussion_r395432315
##########
File path: core/src/main/java/org/apache/calcite/plan/RelTraitDef.java
##########
@@ -147,6 +147,7 @@ public abstract boolean canConvert(
* @param fromRel the RelNode to convert from (with fromTrait)
* @return true if fromTrait can be converted to toTrait
*/
+ @Deprecated // to be removed before 1.24
Review comment:
This can be moved to the JavaDoc?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1863:
[CALCITE-3865] RelCollationTraitDef.canConvert should always return true
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1863: [CALCITE-3865] RelCollationTraitDef.canConvert should always return true
URL: https://github.com/apache/calcite/pull/1863#discussion_r395442252
##########
File path: core/src/main/java/org/apache/calcite/plan/RelTraitDef.java
##########
@@ -147,6 +147,7 @@ public abstract boolean canConvert(
* @param fromRel the RelNode to convert from (with fromTrait)
* @return true if fromTrait can be converted to toTrait
*/
+ @Deprecated // to be removed before 1.24
Review comment:
you mean L148?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1863:
[CALCITE-3865] RelCollationTraitDef.canConvert should always return true
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1863: [CALCITE-3865] RelCollationTraitDef.canConvert should always return true
URL: https://github.com/apache/calcite/pull/1863#discussion_r395456033
##########
File path: core/src/main/java/org/apache/calcite/plan/RelTraitDef.java
##########
@@ -147,6 +147,7 @@ public abstract boolean canConvert(
* @param fromRel the RelNode to convert from (with fromTrait)
* @return true if fromTrait can be converted to toTrait
*/
+ @Deprecated // to be removed before 1.24
Review comment:
I think in the JavaDoc of a deprecated method, there should be a line marked as @deprecated, and provide some instructions for the client (e.g. please use another method instead, or the method will no longer be supported)
So I think line 150 should be moved to the JavaDoc?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1863:
[CALCITE-3865] RelCollationTraitDef.canConvert should always return true
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1863: [CALCITE-3865] RelCollationTraitDef.canConvert should always return true
URL: https://github.com/apache/calcite/pull/1863#discussion_r395456780
##########
File path: core/src/main/java/org/apache/calcite/plan/RelTraitDef.java
##########
@@ -147,6 +147,7 @@ public abstract boolean canConvert(
* @param fromRel the RelNode to convert from (with fromTrait)
* @return true if fromTrait can be converted to toTrait
*/
+ @Deprecated // to be removed before 1.24
Review comment:
Sorry, I noticed this way is common in the Calcite code base. So this is not a problem.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1863:
[CALCITE-3865] RelCollationTraitDef.canConvert should always return true
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1863: [CALCITE-3865] RelCollationTraitDef.canConvert should always return true
URL: https://github.com/apache/calcite/pull/1863#discussion_r395442088
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -1755,7 +1755,7 @@ public RelBuilder aggregate(GroupKey groupKey, Iterable<AggCall> aggCalls) {
newProjects.add(project.getProjects().get(i));
builder.add(project.getRowType().getFieldList().get(i));
}
- r = project.copy(r.getTraitSet(), project.getInput(), newProjects,
+ r = project.copy(cluster.traitSet(), project.getInput(), newProjects,
builder.build());
Review comment:
Not worth it. The bug has been there for years. Logical operator should not have any traitset, including LogicalSort.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1863:
[CALCITE-3865] RelCollationTraitDef.canConvert should always return true
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1863: [CALCITE-3865] RelCollationTraitDef.canConvert should always return true
URL: https://github.com/apache/calcite/pull/1863#discussion_r395484713
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -1755,7 +1755,7 @@ public RelBuilder aggregate(GroupKey groupKey, Iterable<AggCall> aggCalls) {
newProjects.add(project.getProjects().get(i));
builder.add(project.getRowType().getFieldList().get(i));
}
- r = project.copy(r.getTraitSet(), project.getInput(), newProjects,
+ r = project.copy(cluster.traitSet(), project.getInput(), newProjects,
builder.build());
Review comment:
Is LogicalSort useless then?
----------------------------------------------------------------
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
With regards,
Apache Git Services