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.

Sign up for GitHub

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

Jump to bottom

[CALCITE-4392] The operation of checking types equal ignoring null can be more efficient #2256

Merged
merged 1 commit into from
Nov 18, 2020
Merged

[CALCITE-4392] The operation of checking types equal ignoring null can be more efficient #2256

merged 1 commit into from
Nov 18, 2020

Conversation

liyafan82
Copy link
Contributor

danny0405
danny0405 reviewed Nov 12, 2020
View reviewed changes
core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java Outdated
type1.getFullTypeString(),
type2.getFullTypeString(),
expectedResult ? "equal" : "unequal");

Copy link
Contributor

Choose a reason for hiding this comment

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

unequal is not a valid word.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to 'not equal'. Thanks.

Copy link
Contributor

@vlsi vlsi Nov 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Another issue here is that both asserts have the same message, so it is hard to tell which fails.

I would suggest:

    assertThat(
        "SqlTypeUtil.equalSansNullability(typeFactory, " + type1 + ", " + type2 + "), comment: " + comment,
        SqlTypeUtil.equalSansNullability(f.typeFactory, type1, type2), is(expectedResult));
    assertThat(
        "SqlTypeUtil.equalSansNullability(" + type1 + ", " + type2 + "), comment: " + comment,
        SqlTypeUtil.equalSansNullability(type1, type2), is(expectedResult));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable. Revised accordingly. Thank you.

vlsi
vlsi reviewed Nov 12, 2020
View reviewed changes
core/src/test/java/org/apache/calcite/sql/type/SqlTypeUtilTest.java Outdated
RelDataType bigIntType1 =
f.typeFactory.createTypeWithNullability(nullableBigIntType, false);

// different types
Copy link
Contributor

Choose a reason for hiding this comment

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

This should a parameter to compareTypesIgnoringNullability method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess you mean the type factory should be a parameter? Revised accordingly. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean different types should be a comment parameter rather than a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revised. Thanks for your clarification.

@liyafan82 liyafan82 force-pushed the fly_1111_tp branch 2 times, most recently from bd7b5f8 to e20d5b7 Compare November 12, 2020 10:56
chunweilei
chunweilei reviewed Nov 13, 2020
View reviewed changes
Copy link
Contributor

@chunweilei chunweilei left a comment

Choose a reason for hiding this comment

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

LGTM

@rubenada
Copy link
Contributor

LGTM

@chunweilei chunweilei added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Nov 16, 2020
@chunweilei chunweilei merged commit 2ddc836 into apache:master Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vlsi vlsi vlsi left review comments

@danny0405 danny0405 danny0405 left review comments

@chunweilei chunweilei chunweilei approved these changes

Assignees
No one assigned
Labels
LGTM-will-merge-soon Overall PR looks OK. Only minor things left.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@liyafan82 @rubenada @vlsi @danny0405 @chunweilei