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-3214] Add UnionToUnionRule for materialization matching. #1333

Merged
merged 1 commit into from
Aug 16, 2019

Conversation

jinxing64
Copy link
Contributor

@jinxing64 jinxing64 commented Jul 26, 2019

Below materialization matching fails now

  @Test public void testDEV() {
    String sql0 = "select * from \"emps\" where \"empid\" < 300";
    String sql1 = "select * from \"emps\" where \"empid\" > 200";
    checkMaterialize(sql0 + " union all " + sql1, sql1 + " union all " + sql0);
  }

This PR proposes to add a rule for Union matching

@jinxing64 jinxing64 changed the title [CALCITE-3214] Add UnionToUnionRule for materialization matching. [WIP] [CALCITE-3214] Add UnionToUnionRule for materialization matching. Jul 26, 2019
@jinxing64 jinxing64 force-pushed the CALCITE-3214 branch 2 times, most recently from feeb8a7 to b8a6702 Compare July 31, 2019 15:32
@jinxing64 jinxing64 changed the title [WIP] [CALCITE-3214] Add UnionToUnionRule for materialization matching. [CALCITE-3214] Add UnionToUnionRule for materialization matching. Aug 1, 2019
@hsyuan
Copy link
Member

hsyuan commented Aug 8, 2019

What if the inputs of union have different column aliases? The union's output column names are expected to be the same with its first child.

@jinxing64
Copy link
Contributor Author

@hsyuan
Thanks for looking into this ~
After this change -- #1310 , there's no strict column name checking but only column count and column type checking (SubstitutionVisitor::rowTypesAreEquivalent).
Do you think there's strong need to keep the column name same during materialization matching?

List<MutableRel> queryInputs = query.getInputs();
List<MutableRel> targetInputs = target.getInputs();
if (queryInputs.size() == targetInputs.size()) {
while (!queryInputs.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

How about using for loop (i .. n) here? I don't think removing elements in query inputs is necessary. Other than that, looks good to me.

Copy link
Contributor Author

@jinxing64 jinxing64 Aug 16, 2019

Choose a reason for hiding this comment

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

hmm~ let me think. The process of removing elements might be useful/necessary.
Let's make an example. Say Query and MV are like below:

Query:
Union
+--ScanA
+--ScanA
+--ScanB

MV:
Union
+--ScanA
+--ScanB
+--ScanB

Obviously the matching should be failed, but if I implement like below:

if (queryInputs.size() == targetInputs.size()) {
  for (MutableRel rel: queryInputs) {
    if (targetInputs.indexOf(queryInputs.get(0)) == -1) {
      return null;
    }
  }
}
return true;

The matching will succeeded, which is not correct. Though all input scans of query exist in MV, the counts are different.

THX again for review this :)

Copy link
Member

@hsyuan hsyuan Aug 16, 2019

Choose a reason for hiding this comment

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

No, this is what I mean:

if (queryInputs.size() == targetInputs.size()) {
  for (MutableRel rel: queryInputs) {
   int index = targetInputs.indexOf(rel);
    if (index == -1) {
      return null;
    } else {
      targetInputs.remove(index);
    }
  }
}

You don't need to remove twice. Does this make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah~ I got it~ Thanks ~ refine soon.

@jinxing64
Copy link
Contributor Author

jinxing64 commented Aug 16, 2019

Thanks @hsyuan for review this ~
I refined by your comments and squashed the commits :)

Copy link
Member

@hsyuan hsyuan left a comment

Choose a reason for hiding this comment

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

+1

@hsyuan hsyuan added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Aug 16, 2019
@jinxing64
Copy link
Contributor Author

Thanks for approve ~

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
Development

Successfully merging this pull request may close these issues.

2 participants