Skip to content

[CALCITE-3203] When matching materializations, match Project with child of Aggregate#1324

Closed
jinxing64 wants to merge 1 commit intoapache:masterfrom
jinxing64:CALCITE-3203
Closed

[CALCITE-3203] When matching materializations, match Project with child of Aggregate#1324
jinxing64 wants to merge 1 commit intoapache:masterfrom
jinxing64:CALCITE-3203

Conversation

@jinxing64
Copy link
Contributor

In current code, SubstitutionVisitor & MaterializedViewSubstitutionVisitor fail to support below matching:

   query:   Project(projects: [$0, *(2, $1)])
                  Aggregate(groupSet: {0}, groupSets: [{0}], calls: [SUM($1)])
                     Scan(table: [hr, emps])</li>
   target:  Project(projects: [$0, *(2, $1), *(2, $2)])
                  Aggregate(groupSet: {0}, groupSets: [{0}], calls: [SUM($1), COUNT()])
                     Scan(table: [hr, emps])</li>

And below test fails

// MaterializationTest.java
  @Test public void testAggregate() {
    checkMaterialize(
      "select \"deptno\", count(1), 2 * sum(\"empid\") from "
        + "(select * from \"emps\" union all select * from \"emps\")"
        + "group by \"deptno\"",
      "select \"deptno\", 2 * sum(\"empid\") from "
        + "(select * from \"emps\" union all select * from \"emps\")"
        + "group by \"deptno\"");
  }

The reason is that Project&Aggregate are not taken into consideration at the same time in current matching rules.
It might make sense to create a rule of ProjectOnAggregateToProjectOnAggregateUnifyRule to handle such case.

@jinxing64 jinxing64 changed the title wip [CALCITE-3203] When matching materializations, match Project with child of Aggregate Jul 20, 2019
@jinxing64 jinxing64 force-pushed the CALCITE-3203 branch 3 times, most recently from 39233c4 to 45a0393 Compare July 20, 2019 09:44
* Scan(table: [hr, emps])</li>
* </ul>
*/
private static class ProjectOnAggregateToProjectOnAggregateUnifyRule
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can rename the rule name to ProjectAggregateToProjectAggregateUnifyRule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

THX for look into this @hsyuan
I will update soon.

@hsyuan
Copy link
Member

hsyuan commented Aug 22, 2019

Can you rebase on master?

@jinxing64
Copy link
Contributor Author

jinxing64 commented Aug 22, 2019

@hsyuan
Thanks for your kind reminder.

Actually I think this case can be covered by #1384
The matching pattern proposed in this PR is like below:

   query:   Project(projects: [$0, *(2, $1)])
                   Aggregate(groupSet: {0}, groupSets: [{0}], calls: [SUM($1)])
                     Scan(table: [hr, emps])
    target:  Project(projects: [$0, *(2, $1), *(2, $2)])
                   Aggregate(groupSet: {0}, groupSets: [{0}], calls: [SUM($1), COUNT()])
                     Scan(table: [hr, emps])

The matching pattern proposed in #1384 is like below:

 query:   Project(projects: [$0, +($1, 2)])
                 Project(projects: [$1, $3, $4])
                    Rel-A
 target:  Project(projects: [$1, +($3, 2)])
                 Rel-A

I think the second pattern is more general and I added the test case in this PR to #1384
How do you think ? Which one is preferred ?

If #1384 makes sense, it's great if you can shepherd and take a review when you have time. Then I will close this PR.

Thanks ~

Jin

@jinxing64
Copy link
Contributor Author

This change can be covered by #1451
So I'm going to close it now.

@jinxing64 jinxing64 closed this Oct 31, 2019
@jinxing64 jinxing64 deleted the CALCITE-3203 branch October 31, 2019 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants