[CALCITE-3113] Equivalent MutableAggregates with different row types should match with each other. #1310

jinxing64 · 2019-07-09T14:53:47Z

In current code, below test case fails:

  @Test public void testAggregate1() {
    checkMaterialize(
        "select count(*) as c0 from \"emps\" group by \"empid\"",
        "select count(*) as c1 from \"emps\" group by \"empid\"");
  }

java.lang.AssertionError
    at org.apache.calcite.plan.SubstitutionVisitor.go(SubstitutionVisitor.java:504)
    at org.apache.calcite.plan.SubstitutionVisitor.go(SubstitutionVisitor.java:465)
    at org.apache.calcite.plan.MaterializedViewSubstitutionVisitor.go(MaterializedViewSubstitutionVisitor.java:56)
    at org.apache.calcite.plan.RelOptMaterializations.substitute(RelOptMaterializations.java:200)
    at org.apache.calcite.plan.RelOptMaterializations.useMaterializedViews(RelOptMaterializations.java:72)
    at org.apache.calcite.plan.volcano.VolcanoPlanner.registerMaterializations(VolcanoPlanner.java:347)

The reason is that MutableRel self-defined the identity mechanism. Obviously the materialization above should be used.

This PR proposes to do equivalence checking for row types but except for the field names.

I added two test cases in MaterializationTest, without this change, both of them fails.

core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java

julianhyde · 2019-07-09T18:35:33Z

What do we do elsewhere (e.g. in RelOptRules)? Do we check whether row-types are the same modulo field names?

danny0405 · 2019-07-10T06:02:53Z

What do we do elsewhere (e.g. in RelOptRules)? Do we check whether row-types are the same modulo field names?

My concern is the same, in RelOptPlanner, we think 2 rels are equivalent only their rowType and digest are all the same with each other, for row type equivalence, we mean the field names also need to be the same, should we sync this rule for MaterializedView ?

jinxing64 · 2019-07-10T08:58:10Z

@julianhyde @danny0405 THX a lot for review !
In this change, when targetDescendant==target, I add a compensation Project, thus the final replaced has exactly the same row-type as before.
When targetDescendant != target, I take two nodes with same digest but different row-types as equivalent, thus to achieve the matching, but this mismatch of row-types will not be visible in the final result.

danny0405 · 2019-07-15T02:47:41Z

core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java

                if (targetDescendant == target) {
                  // A real substitution happens. We purge the attempted
                  // replacement list and add them into substitution list.
                  // Meanwhile we stop matching the descendants and jump
                  // to the next subtree in pre-order traversal.
+                  assert result1.rowType.equals(result.call.query.rowType)
+                      : Pair.of(result1, result.call.query);


This assert is not that necessary, because it is always true now.

OK， I will remove it.

danny0405 · 2019-07-15T02:53:40Z

core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java

+                  result1 = MutableProject.of(
+                      result.call.query.rowType, result.result, projects);
+                } else {
+                  result1 = result.result;


Instead of new a MutableProject, can we just check the project list of the result row type ? If i'm not mistaken, only if the query and target have same row expressions but different alias can be matched, maybe we should only check the expression for alias.

Sorry, I don't quite get it ~ Do you mean below ?

No need to new a MutableProject, though the row type is different, we just let it go

Add a check/assert for the row type, we allow name mismatch, but other parts of row type should match with each other

Sorry, I don't quite get it ~ Do you mean below ?

No need to new a MutableProject, though the row type is different, we just let it go

Add a check/assert for the row type, we allow name mismatch, but other parts of row type should match with each other

Yeah, that is just what i mean.

danny0405 · 2019-07-15T02:54:45Z

core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java

+                  final RexBuilder rexBuilder = target.cluster.getRexBuilder();
+                  List<RexNode> projects = new ArrayList<>(
+                      rexBuilder.identityProjects(result.call.query.rowType));
+                  result1 = MutableProject.of(


Let's give the result1 a more readable name, how about equivalentResult ?

Yes, that'd better.

danny0405 · 2019-07-15T02:58:26Z

I have give some advices for my understanding. But i'm not sure if it is better or worse when we allow to match of materializedView with different row type. Because our planning rules only consider strict row type match for equivalent rel nodes. Maybe @jcamachor can give a better review.

jinxing64 · 2019-07-15T09:18:49Z

THX, @danny0405 for your comments, I'll update the patch soon.

…should match with each other.

jinxing64 · 2019-07-16T06:08:24Z

@julianhyde
THX for your comments from JIRA. I updated this patch. Please take another look when you have time. In current code, I extracted out a Boolean rowTypesAreEquivalent(MutableRel rel0, MutableRel rel1, Litmus litmus) method for row type compatibility checking.

core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java

rubenada · 2019-07-17T07:38:08Z

LGTM

jinxing64 · 2019-07-17T07:55:01Z

THX @rubenada for approving !

danny0405 · 2019-07-17T09:20:09Z

core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java

+  private boolean rowTypesAreEquivalent(
+      MutableRel rel0, MutableRel rel1, Litmus litmus) {
+    // Validation checking for row type, but except for the field names.
+    assert rel0.rowType.getFieldCount() == rel1.rowType.getFieldCount()


This comment seems redundant, because there is already a java doc comment for the method.

danny0405 · 2019-07-17T09:21:01Z

core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java

+  private boolean rowTypesAreEquivalent(
+      MutableRel rel0, MutableRel rel1, Litmus litmus) {
+    // Validation checking for row type, but except for the field names.
+    assert rel0.rowType.getFieldCount() == rel1.rowType.getFieldCount()


Why we still use this single assert and the other use the litmus ?

THX Danny,
I updated, please check~

jinxing64 · 2019-07-17T10:42:40Z

@danny0405
THX for merging !

…should match with each other(Jin Xing) The MaterializedView now can match with eath other if the row fields types equals but with different alias. close apache#1310

jinxing64 changed the title ~~[CALCITE-3133] Equivalent MutableAggregates with different row types should match with each other.~~ [CALCITE-3113] Equivalent MutableAggregates with different row types should match with each other. Jul 9, 2019

jinxing64 force-pushed the CALCITE-3133 branch 2 times, most recently from 1a3b707 to 52b12b9 Compare July 9, 2019 15:26

rubenada reviewed Jul 9, 2019

View reviewed changes

core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java Outdated Show resolved Hide resolved

rubenada reviewed Jul 9, 2019

View reviewed changes

core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java Outdated Show resolved Hide resolved

jinxing64 force-pushed the CALCITE-3133 branch from 52b12b9 to afd174d Compare July 9, 2019 16:44

danny0405 reviewed Jul 15, 2019

View reviewed changes

[CALCITE-3113] Equivalent MutableAggregates with different row types …

11dda0d

…should match with each other.

jinxing64 force-pushed the CALCITE-3133 branch from a799d11 to 11dda0d Compare July 16, 2019 05:42

rubenada reviewed Jul 16, 2019

View reviewed changes

core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java Outdated Show resolved Hide resolved

Boolean -> boolean

9db24b0

danny0405 reviewed Jul 17, 2019

View reviewed changes

Refine by Danny's comment.

2cd541f

jinxing64 force-pushed the CALCITE-3133 branch from e55e280 to 2cd541f Compare July 17, 2019 09:38

danny0405 approved these changes Jul 17, 2019

View reviewed changes

danny0405 added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jul 17, 2019

danny0405 closed this in 0a87daf Jul 17, 2019

jinxing64 deleted the CALCITE-3133 branch July 17, 2019 10:42

jinxing64 mentioned this pull request Aug 11, 2019

[CALCITE-3214] Add UnionToUnionRule for materialization matching. #1333

Merged

jinxing64 mentioned this pull request Nov 1, 2019

[CALCITE-3469] Typo in SubstitutionVisitor#rowTypesAreEquivalent #1555

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-3113] Equivalent MutableAggregates with different row types should match with each other. #1310

[CALCITE-3113] Equivalent MutableAggregates with different row types should match with each other. #1310

jinxing64 commented Jul 9, 2019 •

edited

Loading

julianhyde commented Jul 9, 2019

danny0405 commented Jul 10, 2019 •

edited

Loading

jinxing64 commented Jul 10, 2019

danny0405 Jul 15, 2019

jinxing64 Jul 15, 2019

danny0405 Jul 15, 2019

jinxing64 Jul 15, 2019

danny0405 Jul 15, 2019

danny0405 Jul 15, 2019

jinxing64 Jul 15, 2019

danny0405 commented Jul 15, 2019

jinxing64 commented Jul 15, 2019

jinxing64 commented Jul 16, 2019

rubenada commented Jul 17, 2019

jinxing64 commented Jul 17, 2019

danny0405 Jul 17, 2019

danny0405 Jul 17, 2019

jinxing64 Jul 17, 2019

jinxing64 commented Jul 17, 2019

[CALCITE-3113] Equivalent MutableAggregates with different row types should match with each other. #1310

[CALCITE-3113] Equivalent MutableAggregates with different row types should match with each other. #1310

Conversation

jinxing64 commented Jul 9, 2019 • edited Loading

julianhyde commented Jul 9, 2019

danny0405 commented Jul 10, 2019 • edited Loading

jinxing64 commented Jul 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danny0405 commented Jul 15, 2019

jinxing64 commented Jul 15, 2019

jinxing64 commented Jul 16, 2019

rubenada commented Jul 17, 2019

jinxing64 commented Jul 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinxing64 commented Jul 17, 2019

jinxing64 commented Jul 9, 2019 •

edited

Loading

danny0405 commented Jul 10, 2019 •

edited

Loading