Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CALCITE-3182] Trim unused fields for plan of materialized-view before matching. #1307

Merged
merged 1 commit into from
Jul 19, 2019

Conversation

jinxing64
Copy link
Contributor

What changes were proposed in this pull request?

In current code, before matching query with materialized-view, unused fields of query is trimmed but materialized-view is not. Thus below simple SQL fails to be matched though query and materialized-
view are exactly the same:

@Test public void testMaterializationAfterTrimingOfUnusedFields() {
  String sql =
    "select \"y\".\"deptno\", \"y\".\"name\", \"x\".\"sum_salary\"\n" +
    "from\n" +
    " (select \"deptno\", sum(\"salary\") \"sum_salary\" from \"emps\" group by \"deptno\") \"x\"\n" +
    " join\n" +
    " \"depts\" \"y\"\n" +
    " on \"x\".\"deptno\"=\"y\".\"deptno\"\n";
    checkMaterialize(sql, sql);
}

Checking CalciteMaterializer https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/prepare/CalciteMaterializer.java#L83 , I think the code intends to do the trimming, but didn't call the method.
Since unused fields of query is trimmed anyway https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java#L995 , thus I think there's no necessity to keep the materialized-view un-trimmed

How was this patch tested?

Added new test.

@danny0405 danny0405 added the discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR label Jul 9, 2019
@jinxing64
Copy link
Contributor Author

@DonnyZone
Thanks a lot for pointing me to https://issues.apache.org/jira/browse/CALCITE-3113 . As the comment I left in CALCITE-3113, my idea is that row-type checking in https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java#L498 is too strict, I think we might just need to assure that the final result from SubstitutionVisitor should have same row-type as before. That's enough.
How do you think ?

@jinxing64 jinxing64 changed the title [CALCITE-3182] Trim unused fields for plan of materialized-view before matching. [WIP][CALCITE-3182] Trim unused fields for plan of materialized-view before matching. Jul 9, 2019
@jinxing64 jinxing64 changed the title [WIP][CALCITE-3182] Trim unused fields for plan of materialized-view before matching. [CALCITE-3182] Trim unused fields for plan of materialized-view before matching. Jul 17, 2019
@jinxing64
Copy link
Contributor Author

As CALCITE-3113 is already resolved by
0a87daf
I will continue working on this PR.
@rubenada @DonnyZone
I updated this PR and tests passed. Please take another look when you have time

Copy link
Contributor

@rubenada rubenada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jinxing64
Copy link
Contributor Author

@rubenada
THX for approving !

Copy link
Contributor

@DonnyZone DonnyZone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@jinxing64
Copy link
Contributor Author

THX @DonnyZone

@rubenada rubenada added LGTM-will-merge-soon Overall PR looks OK. Only minor things left. and removed discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR labels Jul 18, 2019
@rubenada
Copy link
Contributor

@jinxing64 could you please update the commit message and add your "name in parentheses at the end of the message" (as described in the contributing guidelines)

@jinxing64
Copy link
Contributor Author

@rubenada
THX ~ I updated the commit message and sent a email to dev-subscribe@calcite.apache.org for mailing list subscription

@rubenada rubenada merged commit 58b6ad6 into apache:master Jul 19, 2019
@jinxing64
Copy link
Contributor Author

@rubenada
THX for merging !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM-will-merge-soon Overall PR looks OK. Only minor things left.
Projects
None yet
4 participants