[GitHub] [calcite] zabetak commented on a change in pull request #1543: [CALCITE-3455] Redundant rule firing for both logical and physical nodes

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

zabetak commented on a change in pull request #1543: [CALCITE-3455] Redundant rule firing for both logical and physical nodes
URL: https://github.com/apache/calcite/pull/1543#discussion_r345619045
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/rel/rules/SortRemoveConstantKeysRule.java
 ##########
 @@ -44,7 +45,7 @@
 
   private SortRemoveConstantKeysRule() {
     super(
-        operand(Sort.class, any()),
+        operand(LogicalSort.class, any()),
 
 Review comment:
   Again this is a backwards incompatible change. Other than that I am more in favor of creating different instances of the rule rather than limiting the rule to match only LogicalSort. We have seen projects were people create their own set of logical operators. Changing the rule like this makes it impossible for such projects to use the rule.  

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