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-6727] Column uniqueness constrain should only apply to inner… #4088

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joeyutong
Copy link

In the existing test

// RelMetadataTest.java
@Test void testColumnUniquenessForJoinOnLimit1() {
  final String sql = ""
      + "select *\n"
      + "from emp A\n"
      + "join (\n"
      + "  select * from emp\n"
      + "  limit 1) B\n"
      + "on A.empno = B.empno";
  sql(sql)
      .assertThatAreColumnsUnique(bitSetOf(0), is(true))
      .assertThatAreColumnsUnique(bitSetOf(1), is(true))
      .assertThatAreColumnsUnique(bitSetOf(9), is(true))
      .assertThatAreColumnsUnique(bitSetOf(10), is(true))
      .assertThatAreColumnsUnique(bitSetOf(), is(true))
      .assertThatUniqueKeysAre(bitSetOf());
} 

the join result column A.ENAME is considered to be unique because A.EMPNO is the unique key. The test still passes when the query changes to left join, in which case A.ENAME is not guaranteed to be unique.

Copy link
Contributor

@mihaibudiu mihaibudiu left a comment

Choose a reason for hiding this comment

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

Why not add the test?

Copy link
Contributor

@suibianwanwank suibianwanwank left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1193,6 +1193,24 @@ private void checkColumnUniquenessForFilterWithConstantColumns(String sql) {
.assertThatUniqueKeysAre(bitSetOf());
}

@Test void testColumnUniquenessForLeftJoinOnLimit1() {
Copy link
Contributor

Choose a reason for hiding this comment

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

it is customary to add a JavaDoc comment to the test indicating the JIRA issue that is being addressed. This makes it easier for maintainers to understand the rationale for a test. You can find lots of examples in the code about how that is supposed to be structured, please follow the pattern.

Copy link
Author

Choose a reason for hiding this comment

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

Added the comment. Thanks for your reminder

// Joining with a singleton constrains the keys on the other table
final Double rightMaxRowCount = mq.getMaxRowCount(right);
if (rightMaxRowCount != null && rightMaxRowCount <= 1.0) {
leftColumns = leftColumns.union(joinInfo.leftSet());
Copy link
Contributor

Choose a reason for hiding this comment

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

is this correct even if maxRowCount is 0?

Copy link
Author

Choose a reason for hiding this comment

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

It seems to me that the constrain still works even when the build side is empty

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Author

Choose a reason for hiding this comment

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

@mihaibudiu Any more thoughts on this? Please take another look when you have a moment

@mihaibudiu mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Dec 16, 2024
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.

3 participants