[GitHub] [calcite] julianhyde commented on a change in pull request #1130: [CALCITE-2865] FilterProjectTransposeRule generates wrong traitSet when copyFilter/Project is true

[GitBox <gi...@apache.org>]

julianhyde commented on a change in pull request #1130: [CALCITE-2865] FilterProjectTransposeRule generates wrong traitSet when copyFilter/Project is true	
URL: https://github.com/apache/calcite/pull/1130#discussion_r272735658
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/rel/rules/FilterProjectTransposeRule.java
 ##########
 @@ -155,8 +158,12 @@ public void onMatch(RelOptRuleCall call) {
     final RelBuilder relBuilder = call.builder();
     RelNode newFilterRel;
     if (copyFilter) {
-      newFilterRel = filter.copy(filter.getTraitSet(), project.getInput(),
-          simplifyFilterCondition(newCondition, call));
+      final RelNode input = project.getInput();
+      final RelTraitSet traitSet = filter.getTraitSet()
+          .replaceIfs(RelCollationTraitDef.INSTANCE,
 
 Review comment:
   I don't like the inconsistency either.
   
   It would be nice if the default instance of these rules matched Filter, Project and Aggregate of any convention and automatically transformed to Filter, Project and Aggregate of the same convention. I know that's asking a lot. But it would be a user-friendly default for people who don't want to go to the effort of building rule sets and multi-phase planning.
   
   People who are building complex planners would, I'm sure, create instances of those rules that match only one convention.
   
   I'd also love to deprecate the `RelNode.copy` method and all of its variants in sub-classes. That is the reason why these rules behave inconsistently between single-convention and multi-convention mode.
   
   Could we add a method `RelBuilder RelBuilder.withConvention(Convention)`? Thus rather than
   
   ```
   filter.copy(traitSet, input, newCondition)
   ```
   
   you'd write
   
   ```
   relBuilder.withConvention(filter.getConvention)
     .push(input)
     .filter(newCondition)
     .build()
   ```

----------------------------------------------------------------
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