[calcite] branch master updated: [CALCITE-3865] RelCollationTraitDef.canConvert should always return true

[hyuan]

This is an automated email from the ASF dual-hosted git repository.

hyuan pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/calcite.git


The following commit(s) were added to refs/heads/master by this push:
     new e7095a2  [CALCITE-3865] RelCollationTraitDef.canConvert should always 
return true
e7095a2 is described below

commit e7095a23cf6e082077c887588216df3e3862c20d
Author: Haisheng Yuan <h.y...@alibaba-inc.com>
AuthorDate: Thu Mar 19 18:24:29 2020 -0500

    [CALCITE-3865] RelCollationTraitDef.canConvert should always return true
    
    CALCITE-1148 introduced changes to RelCollationTraitDef to fix RelTrait
    conversion bug, but it is just hiding the underlying issue and adding 
redundant
    and unnecessary 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.
    
    Close #1863
---
 .../main/java/org/apache/calcite/plan/RelTraitDef.java    |  1 +
 .../main/java/org/apache/calcite/plan/volcano/RelSet.java |  2 +-
 .../java/org/apache/calcite/rel/RelCollationTraitDef.java | 15 +--------------
 .../main/java/org/apache/calcite/tools/RelBuilder.java    |  2 +-
 4 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/core/src/main/java/org/apache/calcite/plan/RelTraitDef.java 
b/core/src/main/java/org/apache/calcite/plan/RelTraitDef.java
index a2c4704..07207c9 100644
--- a/core/src/main/java/org/apache/calcite/plan/RelTraitDef.java
+++ b/core/src/main/java/org/apache/calcite/plan/RelTraitDef.java
@@ -147,6 +147,7 @@ public abstract class RelTraitDef<T extends RelTrait> {
    * @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
   public boolean canConvert(
       RelOptPlanner planner,
       T fromTrait,
diff --git a/core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java 
b/core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java
index 9477094..789b5b8 100644
--- a/core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java
+++ b/core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java
@@ -205,7 +205,7 @@ class RelSet {
         RelTrait toTrait = to.getTraitSet().getTrait(traitDef);
 
         if (toTrait == null || !traitDef.canConvert(
-            cluster.getPlanner(), fromTrait, toTrait, from)) {
+            cluster.getPlanner(), fromTrait, toTrait)) {
           needsConverter = false;
           break;
         }
diff --git 
a/core/src/main/java/org/apache/calcite/rel/RelCollationTraitDef.java 
b/core/src/main/java/org/apache/calcite/rel/RelCollationTraitDef.java
index 418d5f1..bab96ab 100644
--- a/core/src/main/java/org/apache/calcite/rel/RelCollationTraitDef.java
+++ b/core/src/main/java/org/apache/calcite/rel/RelCollationTraitDef.java
@@ -82,20 +82,7 @@ public class RelCollationTraitDef extends 
RelTraitDef<RelCollation> {
   }
 
   public boolean canConvert(
-       RelOptPlanner planner, RelCollation fromTrait, RelCollation toTrait) {
-    return false;
-  }
-
-  @Override public boolean canConvert(RelOptPlanner planner,
-      RelCollation fromTrait, RelCollation toTrait, RelNode fromRel) {
-    // Returns true only if we can convert.  In this case, we can only convert
-    // if the fromTrait (the input) has fields that the toTrait wants to sort.
-    for (RelFieldCollation field : toTrait.getFieldCollations()) {
-      int index = field.getFieldIndex();
-      if (index >= fromRel.getRowType().getFieldCount()) {
-        return false;
-      }
-    }
+      RelOptPlanner planner, RelCollation fromTrait, RelCollation toTrait) {
     return true;
   }
 }
diff --git a/core/src/main/java/org/apache/calcite/tools/RelBuilder.java 
b/core/src/main/java/org/apache/calcite/tools/RelBuilder.java
index b4283d4..03ecc1c 100644
--- a/core/src/main/java/org/apache/calcite/tools/RelBuilder.java
+++ b/core/src/main/java/org/apache/calcite/tools/RelBuilder.java
@@ -1755,7 +1755,7 @@ public class RelBuilder {
           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());
       }
     }