Skip to content

[CALCITE-6617] TypeCoercion is not applied correctly to comparisons - #3998

Merged
mihaibudiu merged 1 commit into
apache:mainfrom
mihaibudiu:issue6617
Oct 10, 2024
Merged

[CALCITE-6617] TypeCoercion is not applied correctly to comparisons#3998
mihaibudiu merged 1 commit into
apache:mainfrom
mihaibudiu:issue6617

Conversation

@mihaibudiu

@mihaibudiu mihaibudiu commented Oct 7, 2024

Copy link
Copy Markdown
Contributor

TypeCoercion was handling incorrectly most comparisons, leaving them without casts even when the two operands had different types. After this PR comparisons should always compare operands of the same type.

@mihaibudiu

Copy link
Copy Markdown
Contributor Author

@arkanovicz can you please check if this solves the issue you have reported?

RelDataType commonType;
if (SqlTypeUtil.sameNamedType(type1, type2)) {
// First check when one of the types is a string (see JavaDoc on cmmonTypeForBinaryComparison)
commonType = commonTypeForBinaryComparison(type1, type2);

@mihaibudiu mihaibudiu Oct 7, 2024

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.

prior to this change, coercion would not insert casts when comparing say int to double, because they didn't have the same type name. that was a bug.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

agree

@caicancai caicancai left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

overall LGTM

if (SqlTypeUtil.sameNamedType(type1, type2)) {
// First check when one of the types is a string (see JavaDoc on cmmonTypeForBinaryComparison)
commonType = commonTypeForBinaryComparison(type1, type2);
if (commonType == null && SqlTypeUtil.isComparable(type1, type2)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need to judge this SqlTypeUtil.isComparable(type1, type2)?

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.

This is about comparison operators, you'd think that if 2 values with types are comparable, then the values can be compared by being cast to a common type.

@mihaibudiu mihaibudiu changed the title [CALCITE-6617] Regression after CALCITE-2067 which gives a ClassCastException [CALCITE-6617] TypeCoercion is not applied correctly to comparisons Oct 8, 2024
@mihaibudiu
mihaibudiu force-pushed the issue6617 branch 2 times, most recently from 5a7bada to 6a787d0 Compare October 9, 2024 03:26

@NobiGo NobiGo 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.

Strangely, we need to change the numbers written in SQL, which may cause compatibility issues with previous versions.

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
@sonarqubecloud

sonarqubecloud Bot commented Oct 9, 2024

Copy link
Copy Markdown

Comment thread core/src/test/java/org/apache/calcite/test/TypeCoercionTest.java
@mihaibudiu

Copy link
Copy Markdown
Contributor Author

Strangely, we need to change the numbers written in SQL, which may cause compatibility issues with previous versions.

You don't need to modify any SQL programs, but the unparsed programs may look different.

@mihaibudiu
mihaibudiu merged commit 052a5f8 into apache:main Oct 10, 2024
@mihaibudiu
mihaibudiu deleted the issue6617 branch October 10, 2024 01:25
julianhyde pushed a commit to julianhyde/calcite that referenced this pull request Oct 10, 2024
Additional tests in cast.iq (Julian Hyde)

Close apache#3998

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants