[CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol… by twdsilva · Pull Request #2876 · apache/calcite

twdsilva · 2022-08-17T19:59:51Z

…lup view for queries that contain a predicate on the rollup column

sdreynolds · 2022-08-25T21:29:23Z

+        for (RexNode rexNode : project.getProjects()) {
+          if (rexNode instanceof RexCall) {
+            RexCall rexCall = (RexCall) rexNode;
+            if (rexCall.getOperator() instanceof SqlFloorFunction) {


Should this compare based on SqlKind ? https://github.com/apache/calcite/blob/b9c2099ea92a575084b55a206efc5dd341c0df62/core/src/main/java/org/apache/calcite/sql/SqlKind.java

Updated the PR with this change, thanks for the review @sdreynolds.

asolimando · 2022-11-13T11:40:46Z

+          new RelShuttleImpl() {
+            @Override public RelNode visit(LogicalFilter filter) {
+              RexNode condition =
+                  RexUtil.flatten(rexBuilder,
+                      rexBuilder.makeCall(
+                          SqlStdOperatorTable.AND,
+                          filter.getCondition(),
+                          queryCompensationPred));
+              return filter.copy(filter.getTraitSet(), filter.getInput(), condition);
+            }
+
+            @Override public RelNode visit(RelNode other) {
+              if (other instanceof RelSubset) {
+                RelSubset relSubset = (RelSubset) other;
+                return relSubset.getBestOrOriginal().accept(this);
+              } else {
+                return visitChildren(other);
+              }
+            }
+          });
+    } else {
+      rewrittenPlan = relBuilder
+          .push(target)


I think it's worth extracting the RelShuttleImpl to a private class to improve readability

asolimando · 2022-11-13T11:46:28Z

+  @Override public @Nullable  Pair<RelNode, RexNode> rewriteInputView(RelOptRuleCall call,
+      RelNode view, RelNode viewNode, RexBuilder rexBuilder, Pair<RexNode, RexNode> queryPreds,
+      RexNode viewPred) {
+    if (!queryPreds.right.isAlwaysTrue() && viewPred.isAlwaysTrue()) {


queryPreds.right is nullable here, we need to check for queryPreds.right != null before invoking a method to avoid NPE

asolimando · 2022-11-13T11:54:53Z

+                ImplicitViewPredicateShuttle viewPredicateShuttle2 =
+                    new ImplicitViewPredicateShuttle(rexBuilder, rexCall, rexInputRef, true);
+                RexNode viewPredicate = queryPreds.right.accept(viewPredicateShuttle);
+                if (viewPredicateShuttle.isRangeMatched()) {
+                  RexNode viewFilter = queryPreds.right.accept(viewPredicateShuttle2);


We can move the init of the second shuttle inside the if, the only place where it's used.

Suggested change

ImplicitViewPredicateShuttle viewPredicateShuttle2 =

new ImplicitViewPredicateShuttle(rexBuilder, rexCall, rexInputRef, true);

RexNode viewPredicate = queryPreds.right.accept(viewPredicateShuttle);

if (viewPredicateShuttle.isRangeMatched()) {

RexNode viewFilter = queryPreds.right.accept(viewPredicateShuttle2);

RexNode viewPredicate = queryPreds.right.accept(viewPredicateShuttle);

if (viewPredicateShuttle.isRangeMatched()) {

ImplicitViewPredicateShuttle viewPredicateShuttle2 =

new ImplicitViewPredicateShuttle(rexBuilder, rexCall, rexInputRef, true);

RexNode viewFilter = queryPreds.right.accept(viewPredicateShuttle2);

Additionally, let's find a more descriptive name than viewPredicateShuttle, something likeviewPredicateShuttleFilterGenerator?

Possibly let's improve also viewPredicateShuttle, with something like viewPredicateShuttleRangeMatcher?

asolimando · 2022-11-13T11:56:33Z

+          int viewColumnIndex = 0;
+          for (RexNode rexNode : project.getProjects()) {
+            if (rexNode instanceof RexCall) {
+              RexCall rexCall = (RexCall) rexNode;


I feel this is the right spot where an auxiliary method could be extracted, rewriteInputView is too long and does a lot of things, what do you think?

Added a method called getViewAndViewPredicate.

asolimando · 2022-11-13T12:10:33Z

+        RexInputRef rexInputRef,
+        boolean generateViewFilter) {


Nit: we generally don't stack up arguments in Calcite

Suggested change

RexInputRef rexInputRef,

boolean generateViewFilter) {

RexInputRef rexInputRef, boolean generateViewFilter) {

removed the generateViewFilter param as its no longer required

asolimando · 2022-11-13T12:12:52Z

+          RexNode modifiedCall = rexBuilder.makeCall(call.getType(), transformedCallOperator,
+              reverseOperands ? ImmutableList.of(tableInputRefOperand, truncatedLiteral)
+                  : ImmutableList.of(truncatedLiteral, tableInputRefOperand));
+          return modifiedCall;


I guess you use the variable for easing debug, for modern debuggers can step past the return expression, I think we lose clarity and we don't gain much, I suggest to drop the variable and return immediately.

asolimando · 2022-11-13T12:13:41Z

+      this.generateViewFilter = generateViewFilter;
+    }
+
+    private RexNode transformCall(RexCall call, boolean isLowerBound) {


We need to split this method using auxiliary ones, can you rework it to reduce the overall complexity?

added a method adjustComparisonBoundary .

asolimando · 2022-11-13T13:09:19Z

+      boolean reverseOperands = false;
+      if ((literalOperand.getKind() == SqlKind.LITERAL
+          && tableInputRefOperand.getKind() == SqlKind.TABLE_INPUT_REF)
+          || (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+          && tableInputRefOperand.getKind() == SqlKind.LITERAL)) {
+        if (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+            && tableInputRefOperand.getKind() == SqlKind.LITERAL) {


What about simplifying the if condition by introducing some boolean variables like isLeftLiteral, isRightLiteral etc?

Suggested change

boolean reverseOperands = false;

if ((literalOperand.getKind() == SqlKind.LITERAL

&& tableInputRefOperand.getKind() == SqlKind.TABLE_INPUT_REF)

|| (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF

&& tableInputRefOperand.getKind() == SqlKind.LITERAL)) {

if (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF

&& tableInputRefOperand.getKind() == SqlKind.LITERAL) {

final boolean isLeftLiteral = literalOperand.getKind() == SqlKind.LITERAL;

final boolean isRightLiteral = tableInputRefOperand.getKind() == SqlKind.LITERAL;

final boolean isLeftTableInputRef = literalOperand.getKind() == SqlKind.TABLE_INPUT_REF;

final boolean isRightTableInputRef = tableInputRefOperand.getKind() == SqlKind.TABLE_INPUT_REF;

boolean reverseOperands = false;

if (isLeftLiteral && isRightTableInputRef || isLeftTableInputRef && isRightLiteral) {

if (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF

&& tableInputRefOperand.getKind() == SqlKind.LITERAL) {

asolimando · 2022-11-13T13:10:27Z

+          reverseOperands = true;
+        }
+        int predicateIndex = ((RexTableInputRef) tableInputRefOperand).getIndex();
+        if (floorIndex == predicateIndex) {


Here we could probably replace the if/else with a call to an aux method

I added the new method adjustComparisonBoundary to handle the if case here.

asolimando · 2022-11-13T13:12:13Z

+      if (v0 == null) {
+        throw new AssertionError("interpreter returned null for " + v0);
+      }
+      Comparable v1 = RexInterpreter.evaluate(literalOperand, Collections.emptyMap());
+      if (v1 == null) {
+        throw new AssertionError("interpreter returned null for " + v1);
+      }


v0 and v1 are always null when you build the assertion message, replace v0/v1 with truncatedLiteral and literalOperand (i.e., what you were evaluating when null was returned)

Moreover, sentences should start with a capital letter

asolimando · 2022-11-13T13:14:47Z

+      // Since the view contains a FLOOR() if the query does a > or <= and the operand is already
+      // a FLOORED value we have to shift the value to the next higher or lower floored value
+      // If the query contains a >= or < we don't need to shift the modified value


Can you add an example here?

asolimando · 2022-11-13T13:16:41Z

+    }
+
+    public boolean isRangeMatched() {
+      return lowerBound != 0L || upperBound != 0L && lowerBound != upperBound;


Don't you mean the following?

Suggested change

return lowerBound != 0L || upperBound != 0L && lowerBound != upperBound;

return (lowerBound != 0L || upperBound != 0L) && lowerBound != upperBound;

asolimando · 2022-11-13T13:21:13Z

        .ok();
  }

+  @Test void testAggregateMaterializationAggregateFuncsRange1() {


Please find better names for tests, if you have multiple tests, they are covering something specific each, so why don't put that in the test name? This is true for all tests1...n in the PR

changed test names to be more descriptive

asolimando · 2022-11-13T13:23:34Z

+          // ignore predicates where the type isn't a timestamp
+          return rexBuilder.makeLiteral(true);
+        }
+        RexNode lb = rexBuilder.makeTimestampLiteral((TimestampString) r.lowerEndpoint(), 0);


Please avoid cryptic names like lb, ub, there is already a lot of complex logic in these methods, let's try to help the code reader with more explicit names like lowerBound and upperBound.

They are surely longer, but nowadays we have IDEs for that :)

asolimando · 2022-11-13T13:38:41Z

+      this.generateViewFilter = generateViewFilter;
+    }
+
+    private RexNode transformCall(RexCall call, boolean isLowerBound) {


I rename transformCall into adjustComparisonBoundary, transformComparisonBoundary or something closer to what this method is actually doing

asolimando · 2022-11-13T13:42:58Z

+        Sarg modSarg = Sarg.of(RexUnknownAs.UNKNOWN,
+            ImmutableRangeSet.of(Range.closedOpen(lbTimestampString, ubTimestampString)));
+        RexNode searchArgumentLiteral = rexBuilder.makeSearchArgumentLiteral(modSarg, type);
+        RexNode res = rexBuilder.makeCall(SqlStdOperatorTable.SEARCH, tableInputRefOperand,


What about dropping res and returning the expressions?

asolimando · 2022-11-13T14:03:20Z

    default:
-      return TimeUnitRange.DAY;
+      return SqlFunctions.ceil(v, unit.startUnit.multiplier.longValue());


If the default case is just for ceil, I'd rather cover it explicitly

asolimando · 2022-11-13T14:05:06Z

        is(false));
  }

+  private void helpTestFloorCeil(String timestampString, String expectedFloorString,


assertFloorCeil or checkFloorCeil?

asolimando · 2022-11-13T14:06:47Z

+  private void helpTestFloorCeil(String timestampString, String expectedFloorString,
+      String expectedCeilString, TimeUnitRange timeUnitRange) {
+    final RexLiteral timestampLiteral = rexBuilder.makeTimestampLiteral(
+        new TimestampString(timestampString), 3);


Is precision 3 always appropriate?

Yes because the input timestamp string is always contains milliseconds.

asolimando

@twdsilva thanks for the PR, I think it's a very useful feature, I still share the doubts that @julianhyde raised in a jira comment (copying a filter into the view seems hacky and does not always seem safe) and that @zabetak raised in the ML (isn't there a way to modify the query to match the view, instead of changing the view and making view matching more complex?).

In the same ML discussion, you have raised a question on this point, I don't have an answer to that myself, but I feel that the we all need to agree that we are going into the right direction before being able to merge the PR.

asolimando · 2022-11-13T14:08:33Z

+    long expectedFloorValue = new TimestampString(expectedFloorString).getMillisSinceEpoch();
+    assertThat(eval(flooredLiteral), is(expectedFloorValue));
+
+    RexNode ceiledLiteral = rexBuilder.makeCall(SqlStdOperatorTable.CEIL,


Please be consistent with the final modifier, since you have added it on some other vars, if you omit it here, the reader thinks it's going to mutate, while this is not the case

added final modifier here

asolimando · 2022-11-13T14:12:15Z

+        TimeUnitRange.HOUR);
+    helpTestFloorCeil(timestampString, "2011-07-20 00:00:00", "2011-07-21 00:00:00",
+        TimeUnitRange.DAY);
+    helpTestFloorCeil("2011-07-20 12:34:59.078", "2011-07-17 00:00:00", "2011-07-24 00:00:00",


The start of week here is on Sunday, which is locale dependant, is there a way to make this locale dependant? If not, why would it be OK as is?

From the comment on CALCITE-3412 I think FLOOR(timestamp TO WEEK) will always round down to the previous Sunday.
https://issues.apache.org/jira/browse/CALCITE-3412?focusedCommentId=16953504&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16953504

asolimando · 2022-11-13T14:18:52Z

+  }
+
+  @Test void testAggregateMaterializationAggregateFuncsRange31() {
+    // test using multiple views


can you improve the comment to match the explanation of the tests above which is of great help? Notably, please cover why mv0 is picked over mv1 and sketch what happens when multiple views are available

Added a more descriptive comment.

asolimando · 2022-11-13T14:19:29Z

+        .ok();
+  }
+
+


Nit: remove the extra newline here

asolimando · 2022-11-13T14:21:34Z

+          long lowerBound = ((TimestampString) r.lowerEndpoint()).getMillisSinceEpoch();
+          long upperBound = ((TimestampString) r.upperEndpoint()).getMillisSinceEpoch();
+          // only used for a range less than one day
+          selectivity += (upperBound - lowerBound) / (Double.valueOf(DateTimeUtils.MILLIS_PER_DAY));


Unnecessary boxing, it can be simplified as:

Suggested change

selectivity += (upperBound - lowerBound) / (Double.valueOf(DateTimeUtils.MILLIS_PER_DAY));

selectivity += (upperBound - lowerBound) / ((double) DateTimeUtils.MILLIS_PER_DAY);

asolimando · 2022-11-13T14:34:25Z

        .checkingThatResultContains("EnumerableTableScan(table=[[hr, events]])")
        .ok();
  }



Could you add one or more tests explicitly covering your claim that the behaviour is unchanged when we have more complex cases (a predicate already exists, we have a predicate in a join, etc.)?

I have added testAggregateViewWithPredicate to test when a predicate already exists in the view and testJoinViewWithoutPredicate, testJoinViewWithPredicateSameAsQueryPredicate, testJoinViewWithPredicateDifferentThanQueryPredicate to test views that have a join with predicate.

twdsilva · 2022-12-03T03:58:32Z

@twdsilva thanks for the PR, I think it's a very useful feature, I still share the doubts that @julianhyde raised in a jira comment (copying a filter into the view seems hacky and does not always seem safe) and that @zabetak raised in the ML (isn't there a way to modify the query to match the view, instead of changing the view and making view matching more complex?).

In the same ML discussion, you have raised a question on this point, I don't have an answer to that myself, but I feel that the we all need to agree that we are going into the right direction before being able to merge the PR.

Thank you @asolimando for the review.
We have a use case where we maintain materialized preaggregated cubes that store data rolled up to the UTC day or hour. The queries in our system always have a time range predicate that doesn't neccesarily align on the UTC day or boundary. We would like to use the materialized cube(s) table whenever possible as they contain a lot fewer rows than the base table. Here is an example.
I could not figure how to get MaterializedViewAggregateRule to handle the case when a view contains a rollup (has a group by with a FLOOR(col)) other than rewriting the view to include a predicate based on the query predicate. The existing code that uses the union operator works if we include this generated predicate. The other main change is to map FLOOR(col) to col which is valid because the predicate added to the view is aligned on the floored value of the query predicate. If there is a cleaner way to make this work I would appreciate any pointers.

[CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…

c64256e

…lup view for queries that contain a predicate on the rollup column

twdsilva mentioned this pull request Aug 22, 2022

Support time ranges that don't start/end on the UTC date boun… twilio/calcite-kudu#78

Merged

1 task

sdreynolds reviewed Aug 25, 2022

View reviewed changes

asfgit force-pushed the main branch from 9fc50f2 to e2f949d Compare September 10, 2022 16:37

twdsilva force-pushed the CALCITE-5240 branch 2 times, most recently from c761219 to 509149c Compare September 23, 2022 20:01

Address code review feedback

381459c

twdsilva force-pushed the CALCITE-5240 branch from 509149c to 381459c Compare September 23, 2022 20:28