[GitHub] [calcite] michaelmior commented on a change in pull request #2188: [CALCITE-4296] Materialization recognition fail, cannot match Calc on…

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

michaelmior commented on a change in pull request #2188:
URL: https://github.com/apache/calcite/pull/2188#discussion_r609648990



##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1055,12 +1056,36 @@ private TrivialRule() {
       super(any(MutableRel.class), any(MutableRel.class), 0);
     }
 
-    @Override public @Nullable UnifyResult apply(UnifyRuleCall call) {
+    @Override
+    public @Nullable UnifyResult apply(UnifyRuleCall call) {
       if (call.query.equals(call.target)) {
         return call.result(call.target);
       }
+      if (isSpa(call.target) && MutableRels.descendants(call.query)
+          .containsAll(MutableRels.descendants(call.target))) {
+        return call.result(call.query);
+      }
       return null;
     }
+
+    private boolean isSpa(MutableRel rel) {

Review comment:
       What does isSpa do? I'm not sure what Spa is in this context so it would help to have a more descriptive name.

##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1055,12 +1056,36 @@ private TrivialRule() {
       super(any(MutableRel.class), any(MutableRel.class), 0);
     }
 
-    @Override public @Nullable UnifyResult apply(UnifyRuleCall call) {
+    @Override
+    public @Nullable UnifyResult apply(UnifyRuleCall call) {
       if (call.query.equals(call.target)) {
         return call.result(call.target);
       }
+      if (isSpa(call.target) && MutableRels.descendants(call.query)
+          .containsAll(MutableRels.descendants(call.target))) {
+        return call.result(call.query);
+      }
       return null;
     }
+
+    private boolean isSpa(MutableRel rel) {
+      if (rel.getParent() != null) {
+        return false;
+      }
+      if (rel instanceof MutableCalc) {
+        MutableCalc topCalc = (MutableCalc) rel;
+        if (topCalc.getInput() instanceof MutableAggregate) {
+          MutableAggregate aggregate = (MutableAggregate) topCalc.getInput();
+          if (aggregate.getInput() instanceof MutableCalc) {
+            MutableCalc bottomCalc = (MutableCalc) aggregate.getInput();
+            if (bottomCalc.getInput() instanceof MutableScan) {
+              return true;
+            }
+          }
+        }
+      }

Review comment:
       Instead of this deep nested if, I think it might be easier to follow if you just returned false when any of these conditions fail and then everything could be flattened.

##########
File path: core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java
##########
@@ -609,16 +609,16 @@
         .ok();
   }
 
-  @Test void testAggregateOnProjectAndFilter() {
+  @Test void testNoCalcOnAggregate() {

Review comment:
       Is there any reason this existing test is removed instead of making a new one?




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