Skip to content

[CALCITE-4241] Some improvements to metadata query - #2146

Merged
chunweilei merged 1 commit into
apache:masterfrom
liyafan82:fly_0910_md
Sep 21, 2020
Merged

[CALCITE-4241] Some improvements to metadata query#2146
chunweilei merged 1 commit into
apache:masterfrom
liyafan82:fly_0910_md

Conversation

@liyafan82

@liyafan82 liyafan82 commented Sep 11, 2020

Copy link
Copy Markdown
Contributor
  1. For {{RelMdColumnUniqueness#areColumnsUnique(RelSubset, RelMetadataQuery,
    ImmutableBitSet, boolean)}}, we can return early as we encounter the first null.

  2. For {{RelMdDistinctRowCount#getDistinctRowCount(Values, RelMetadataQuery,
    ImmutableBitSet, RexNode)}}, we can get the distinct row count accurately, instead of guessing that half of the elements are distinct.

  3. RelMdUtil#areColumnsDefinitelyUniqueWhenNullsFiltered could be implemented in a more efficient way.

return RelMdUtil.areColumnsDefinitelyUniqueWhenNullsFiltered(mq, leftRel,
return RelMdUtil.areColumnsDefinitelyUnique(mq, leftRel,
joinInfo.leftSet());
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

RelMdUtil#areColumnsDefinitelyUniqueWhenNullsFiltered has different implementation from RelMdUtil#areColumnsDefinitelyUnique. The former one would ignore null while the other one not. Why do you say they have same behavoirs?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@chunweilei Thanks a lot for your careful review. They are different indeed.
However, areColumnsDefinitelyUniqueWhenNullsFiltered methods could be implemented in a more efficient way.

+ ") t(c1, c2, c3)");
final RelMetadataQuery mq = rel.getCluster().getMetadataQuery();

ImmutableBitSet groupKey = ImmutableBitSet.of(0, 1, 2);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add a test case which contains null literal?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure. Cases for null literals are added. Thanks for your good suggestion.

@chunweilei chunweilei added needs-a-final-review This patch looks good from at least one PMC or committer, but still needs a specialist final review LGTM-will-merge-soon Overall PR looks OK. Only minor things left. labels Sep 14, 2020

@chunweilei chunweilei left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@chunweilei chunweilei removed the needs-a-final-review This patch looks good from at least one PMC or committer, but still needs a specialist final review label Sep 15, 2020
@chunweilei
chunweilei merged commit e7ac26d into apache:master Sep 21, 2020
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