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-4683] IN-list converted to JOIN throws type mismatch exception #2454

Merged
merged 1 commit into from Feb 17, 2022
Merged

[CALCITE-4683] IN-list converted to JOIN throws type mismatch exception #2454

merged 1 commit into from Feb 17, 2022

Conversation

hannerwang
Copy link

No description provided.

@hannerwang hannerwang force-pushed the InToListWithNewProjectCorrection branch from 65b3187 to d742d76 Compare January 26, 2022 11:59
@hannerwang hannerwang changed the title [CALCITE-4683] In-list to join with new project generated cause replaced root leaves property to be missing [CALCITE-4683] In-list to join causes field datatypes not matched Jan 26, 2022
asolimando
asolimando requested changes Feb 11, 2022
View reviewed changes
Copy link
Member

@asolimando asolimando left a comment

Choose a reason for hiding this comment

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

The PR looks good, left few comments. Regarding the test failure, it's a known issue, sometimes Druid tests hang, don't worry about that.

core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java Outdated Show resolved Hide resolved
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java Outdated Show resolved Hide resolved
core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java Outdated Show resolved Hide resolved
@hannerwang hannerwang changed the title [CALCITE-4683] In-list to join causes field datatypes not matched [CALCITE-4683] IN-list converted to JOIN throws type mismatch exception Feb 15, 2022
asolimando
asolimando approved these changes Feb 15, 2022
View reviewed changes
Copy link
Member

@asolimando asolimando left a comment •
edited

Choose a reason for hiding this comment

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

LGTM @hannerwang, will merge in the next days if nobody objects
EDIT: I just noticed there is a test failure for continuous-integration/appveyor/pr, can you look into that?

Test failure looks unrelated, it's CassandraAdapterDataTypesTest failing, I will open a ticket to followup on that.

@asolimando asolimando added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Feb 15, 2022
@hannerwang
Copy link
Author

hannerwang commented Feb 15, 2022 •
edited

Thanks @asolimando!

NobiGo
NobiGo approved these changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@NobiGo NobiGo left a comment

Choose a reason for hiding this comment

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

LGTM

@asolimando
Copy link
Member

Thanks @NobiGo for the review. @hannerwang, can you squash the commits into a single one? Then I will merge.

@hannerwang
Copy link
Author

Hi asolimando, do you mean that I squash them at local repository and force push it? I don't know which could be better between 'rebase then force push' and 'squash and merge', so I need confirm it.

@asolimando
Copy link
Member

asolimando commented Feb 16, 2022 •
edited

Yes, you can squash and force push because the review process is now over (we ask not to do it when the review is still ongoing)
EDIT: the commit message will also need to be updated in order to reflect the change of the JIRA ticket title

@hannerwang hannerwang force-pushed the InToListWithNewProjectCorrection branch from c281057 to ca6983f Compare February 17, 2022 02:36
@hannerwang
Copy link
Author

Thanks @asolimando .

@asolimando asolimando merged commit 57ad4ff into apache:master Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asolimando asolimando asolimando approved these changes

@NobiGo NobiGo NobiGo 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
3 participants
@hannerwang @asolimando @NobiGo