[CALCITE-4779] GroupByList contains constant literal, materialized view recognition failed #2529

xy2953396112 · 2021-09-19T08:21:29Z

https://issues.apache.org/jira/projects/CALCITE/issues/CALCITE-4779

xy2953396112 · 2021-09-19T08:58:35Z

@julianhyde
Please help review this pr,thank you.

asolimando

The PR LGTM, I was thinking that tests could cover also constants between column names, and after them, instead of only preceding them.

For instance something like: group by $col1, 1, $col2, group by 1, $col1, $col2 and group by 1, $col1, $col2

core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java

wojustme · 2021-09-20T07:05:21Z

LGTM~

core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java

yanlin-Lynn · 2021-09-20T08:41:25Z

Can you use a varchar constant, not integer constant in the group by list.

asolimando · 2021-09-20T13:43:44Z

@xy2953396112 please avoid squashing while the review process is on-going, old and new changes cannot be distinguished anymore, and reviewers can't just look at the "newer" part.

xy2953396112 · 2021-09-20T13:49:27Z

Can you use a varchar constant, not integer constant in the group by list.

ok.Please review again,thanks.

wojustme · 2021-09-20T13:54:23Z

Can you use a varchar constant, not integer constant in the group by list.
Hi @yanlin-Lynn
Should we need case of group by varchar literal?
In query's relnode, group by varchar literal is the same to group by integer literal.
They are all belong to the pattern of AggregateOnCalc.

xy2953396112 · 2021-09-20T13:54:44Z

@xy2953396112 please avoid squashing while the review process is on-going, old and new changes cannot be distinguished anymore, and reviewers can't just look at the "newer" part.

ok.Please help review, thanks a lot.

asolimando · 2021-09-20T14:24:56Z

Can you use a varchar constant, not integer constant in the group by list.
Hi @yanlin-Lynn
Should we need case of group by varchar literal?
In query's relnode, group by varchar literal is the same to group by integer literal.
They are all belong to the pattern of AggregateOnCalc.

As a general comment, test cases should be crafted with the expected behaviour in mind, not the concrete implementation.

It might seem trivial in the current state of the implementation, but tomorrow a change could break this assumption (maybe simply by mistake), and the test case would capture it.

Of course there must be a reasonable chance for this to happen, I am not saying that this is necessarily the case here.

julianhyde · 2021-09-21T00:53:35Z

I second @asolimando's point about merging. When a PR is under review, don't rebase (unless asked to), don't squash commits, don't amend commits, just add new commits. It makes things easier for reviewers. We know how to do git diff HEAD~5 if we need to.

I think it would be useful if you describe in brief a javadoc sentence the particular feature that each each test is testing. It's difficult to capture this in the test name - for example, the test case "testGroupSetMultipleLiteralsOrder" contains neither GROUPING SET nor ORDER BY. I understand from reading the test that you are interested in a query that contains literals and the literals are in a different order in the SELECT clause to the GROUP BY clause.

I'm not sure that this bug requires 70 lines of new tests, because the fix simply removes literals. Since all tests use the same MV, I would compress them all into one method.

The one case you have not tested is a query that groups by a literal and no columns, e.g. GROUP BY 'a'. The semantics are non-obvious when the table is empty. It warrants its own test.

wojustme · 2021-09-21T03:30:49Z

Can you use a varchar constant, not integer constant in the group by list.
Hi @yanlin-Lynn
Should we need case of group by varchar literal?
In query's relnode, group by varchar literal is the same to group by integer literal.
They are all belong to the pattern of AggregateOnCalc.

As a general comment, test cases should be crafted with the expected behaviour in mind, not the concrete implementation.

It might seem trivial in the current state of the implementation, but tomorrow a change could break this assumption (maybe simply by mistake), and the test case would capture it.

Of course there must be a reasonable chance for this to happen, I am not saying that this is necessarily the case here.

Ok, got it. Thanks for for your advice. @asolimando

xy2953396112 · 2021-09-22T10:56:41Z

@yanlin-Lynn @julianhyde @asolimando
I have updated the code.Any comments?

asolimando

I re-confirm my approval after checking the latest changes, thanks for addressing the comments!

xy2953396112 · 2021-09-23T06:43:52Z

I re-confirm my approval after checking the latest changes, thanks for addressing the comments!

Thanks for review.

yanlin-Lynn · 2021-09-26T09:34:05Z

core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java

+  /** Test case for
+   * <a href="https://issues.apache.org/jira/browse/CALCITE-4779">[CALCITE-4779]
+   * GroupingSet contains {@link RexLiteral}, materialized view recognition failed</a>. */
+  @Test void testGroupingSetLiteral() {


It's not grouping set, it's "constant literal" in group by list. right?

yanlin-Lynn · 2021-09-26T13:18:15Z

core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java

@@ -1691,6 +1691,44 @@ private void checkSatisfiable(RexNode e, String s) {
            + "EnumerableTableScan(table=[[hr, MV0]])")).ok();
  }

+  /** Test case for
+   * <a href="https://issues.apache.org/jira/browse/CALCITE-4779">[CALCITE-4779]
+   * GroupSet contains {@link RexLiteral}, materialized view recognition failed</a>. */


Not GroupSet, it's group by list

yanlin-Lynn · 2021-09-26T13:19:09Z

core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java

+    final String mv1 = ""
+        + "select \"deptno\", \"empid\"\n"
+        + "from \"emps\"\n"
+        + "group by \"deptno\", \"empid\"";


mv0 and mv1 is the same.

thanks,update the code.

…ew recognition failed (xzh)

yanlin-Lynn · 2021-09-27T08:14:35Z

LGTM

asolimando reviewed Sep 19, 2021

View reviewed changes

core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java Outdated Show resolved Hide resolved

xy2953396112 force-pushed the CALCITE_4779 branch 2 times, most recently from 1debc97 to 068acd4 Compare September 20, 2021 00:01

xy2953396112 changed the title ~~[CALCITE_4779] GroupSet contains constant, materialized view recognit…~~ [CALCITE_4779] GroupSet contains literal, materialized view recognit… Sep 20, 2021

asolimando reviewed Sep 20, 2021

View reviewed changes

core/src/test/java/org/apache/calcite/test/MaterializedViewSubstitutionVisitorTest.java Outdated Show resolved Hide resolved

xy2953396112 force-pushed the CALCITE_4779 branch from 068acd4 to 9916a7b Compare September 20, 2021 13:37

xy2953396112 force-pushed the CALCITE_4779 branch from 9916a7b to 4141459 Compare September 20, 2021 14:34

asolimando approved these changes Sep 20, 2021

View reviewed changes

xy2953396112 force-pushed the CALCITE_4779 branch from e6ef436 to b47a264 Compare September 22, 2021 02:14

asolimando approved these changes Sep 22, 2021

View reviewed changes

xy2953396112 force-pushed the CALCITE_4779 branch 2 times, most recently from 1e82bbd to ba3bcab Compare September 22, 2021 12:45

xy2953396112 changed the title ~~[CALCITE_4779] GroupSet contains literal, materialized view recognit…~~ [CALCITE-4779] GroupSet contains literal, materialized view recognit… Sep 23, 2021

xy2953396112 force-pushed the CALCITE_4779 branch 2 times, most recently from 048fd15 to 7814935 Compare September 25, 2021 02:08

yanlin-Lynn reviewed Sep 26, 2021

View reviewed changes

xy2953396112 force-pushed the CALCITE_4779 branch from 7814935 to fd2fa0c Compare September 26, 2021 10:03

yanlin-Lynn reviewed Sep 26, 2021

View reviewed changes

[CALCITE-4779] GroupByList contains constant literal, materialized vi…

b3d6774

…ew recognition failed (xzh)

xy2953396112 force-pushed the CALCITE_4779 branch from fd2fa0c to b3d6774 Compare September 26, 2021 13:50

xy2953396112 changed the title ~~[CALCITE-4779] GroupSet contains literal, materialized view recognit…~~ [CALCITE-4779] GroupByList contains constant literal, materialized view recognition failed Sep 26, 2021

yanlin-Lynn added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Sep 27, 2021

yanlin-Lynn merged commit 219e41e into apache:master Sep 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-4779] GroupByList contains constant literal, materialized view recognition failed #2529

[CALCITE-4779] GroupByList contains constant literal, materialized view recognition failed #2529

xy2953396112 commented Sep 19, 2021 •

edited

xy2953396112 commented Sep 19, 2021

asolimando left a comment

wojustme commented Sep 20, 2021

yanlin-Lynn commented Sep 20, 2021

asolimando commented Sep 20, 2021

xy2953396112 commented Sep 20, 2021

wojustme commented Sep 20, 2021 •

edited

xy2953396112 commented Sep 20, 2021

asolimando commented Sep 20, 2021

julianhyde commented Sep 21, 2021 •

edited

wojustme commented Sep 21, 2021

xy2953396112 commented Sep 22, 2021 •

edited

asolimando left a comment

xy2953396112 commented Sep 23, 2021

yanlin-Lynn Sep 26, 2021

yanlin-Lynn Sep 26, 2021

xy2953396112 Sep 27, 2021

yanlin-Lynn Sep 26, 2021

xy2953396112 Sep 27, 2021

yanlin-Lynn commented Sep 27, 2021

[CALCITE-4779] GroupByList contains constant literal, materialized view recognition failed #2529

[CALCITE-4779] GroupByList contains constant literal, materialized view recognition failed #2529

Conversation

xy2953396112 commented Sep 19, 2021 • edited

xy2953396112 commented Sep 19, 2021

asolimando left a comment

Choose a reason for hiding this comment

wojustme commented Sep 20, 2021

yanlin-Lynn commented Sep 20, 2021

asolimando commented Sep 20, 2021

xy2953396112 commented Sep 20, 2021

wojustme commented Sep 20, 2021 • edited

xy2953396112 commented Sep 20, 2021

asolimando commented Sep 20, 2021

julianhyde commented Sep 21, 2021 • edited

wojustme commented Sep 21, 2021

xy2953396112 commented Sep 22, 2021 • edited

asolimando left a comment

Choose a reason for hiding this comment

xy2953396112 commented Sep 23, 2021

yanlin-Lynn Sep 26, 2021

Choose a reason for hiding this comment

yanlin-Lynn Sep 26, 2021

Choose a reason for hiding this comment

xy2953396112 Sep 27, 2021

Choose a reason for hiding this comment

yanlin-Lynn Sep 26, 2021

Choose a reason for hiding this comment

xy2953396112 Sep 27, 2021

Choose a reason for hiding this comment

yanlin-Lynn commented Sep 27, 2021

xy2953396112 commented Sep 19, 2021 •

edited

wojustme commented Sep 20, 2021 •

edited

julianhyde commented Sep 21, 2021 •

edited

xy2953396112 commented Sep 22, 2021 •

edited