[CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow #3522

mihaibudiu · 2023-11-14T04:51:34Z

This also fixes a few more tests which were disabled in SqlOperatorTests.
I still have to handle casts to decimal values, but hopefully this fixes the behavior of casts to integer types.
This behavior also has to be documented, so a future PR will describe in more details how Calcite evaluates casts.

mihaibudiu · 2023-11-14T17:00:50Z

The CI failure is from the Druid tests, so I guess this PR is ready for review.

zstan · 2023-11-18T12:32:51Z

I already realize the custom logic for such a cases, you can take only tests, check:
https://github.com/apache/ignite-3/pull/2786/files#diff-4ddeb0625d1cc7604c3e33e3d57ca209307a8f3e5b6aea01bee459ce06963870R675
https://github.com/apache/ignite-3/pull/2786/files#diff-ef8a24b20a4922cd2b5e92e466e3a12263444ec6887d0d87d8c885359033cf76R426

mihaibudiu · 2023-11-19T18:27:19Z

These tests are very useful. They cover many more cases than this PR handles. Here we only do casts.

zstan · 2023-11-22T05:25:37Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+        ".* out of range", true);
+    f.checkFails("SELECT cast(1e60+30 as int)",
+        ".* out of range", true);
+    f.checkFails("SELECT cast(1e60+30 as bigint)",


what about: SELECT CAST(2147483648 as BIGINT) ?

These cases should be covered by the test testCastExactNumericLimits, where I re-enabled some of the tests that were disabled.

Well, this cast in particular should not fail, so I guess it's not covered by the NumericLimits test.
I have checked that this test would pass.
Are you suggesting adding tests where I use the numeric limits for a type (INT) when casting to a different type (BIGINT)?

mihaibudiu · 2023-12-27T18:37:57Z

This is a bugfix PR, can someone please review it?

zstan · 2023-12-29T07:09:58Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+   * Explicit cast from expression to numeric type doesn't check overflow</a>. */
+  @Test public void testOverflow() {
+    final SqlOperatorFixture f = fixture();
+    f.checkFails("SELECT cast(100+30 as tinyint)",


as for me - it`s hard to parse such a tests.

Suggested change

f.checkFails("SELECT cast(100+30 as tinyint)",

f.checkFails(String.format("SELECT cast(%d+30 as tinyint)", Byte.MAX_VALUE),

and the same for all near

what about implicit casts, is it all ok there ? or it out of scope ?
SELECT 9223372036854775807 + 1;
SELECT 2147483647 + 1;

This PR is only about casts. I plan a follow up PR about arithmetic, which has been broken for many years. There are many disabled tests because of this. That PR will handle the expressions you describe.

Regarding implicit casts: in a normal compiler, after validation the compiler should convert all implicit casts into explicit casts. This leaves many fewer cases to worry about for the subsequent stages. Unfortunately it doesn't look like Calcite works this way. I don't know enough about validation to promise I will fix this part.

However, implicit casts usually convert values from a narrow type to a wider type, so they should not be a cause of runtime failures.

But only writing lots of tests will ensure that all cases are properly handled.

mihaibudiu · 2023-12-29T22:38:59Z

@zstan I have rewritten the tests you suggested to make the intent clearer.
Thank you for the review, please let me know if you have more suggestions.
Note that this PR does not intend to handle casts to decimal types, that will be a separate PR.
It does not handle arithmetic overflow either, that will be yet another PR.

mihaibudiu · 2024-01-11T22:02:09Z

@zstan, I would like to merge this PR, since the fixes I plan to address depend on these being merged.
Please let me know if you have more comments. I will interpret silence as an approval if you don't mind.

zstan · 2024-01-14T16:25:13Z

@mihaibudiu honestly, overall looks good but as for me this issue need additional eyes here, can you found one more reviewer on dev list ?

asolimando

LGTM, few comments but nothing major

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

asolimando · 2024-01-16T09:56:36Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

        f.checkCastFails("'" + numeric.maxOverflowNumericString + "'",
-            type, OUT_OF_RANGE_MESSAGE, true, castType);
+            type, "Number has wrong format.*", true, castType);


Nit: maybe we can introduce a variable WRONG_FORMAT_MESSAGE in the same spirit of OUT_OF_RANGE_MESSAGE and others?

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

snuyanzin · 2024-01-16T10:47:37Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+    f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)", Byte.MAX_VALUE),
+        ".* out of range", true);


I tried to see what happens if I locally change expectedError to something else...
Surprisingly the tests continue passing
it seems that the reason is that this method is called

calcite/testkit/src/main/java/org/apache/calcite/test/SqlOperatorFixtureImpl.java

Lines 157 to 171 in 0bec957

@Override public void checkFails(StringAndPos sap, String expectedError,

boolean runtime) {

final String sql = "values (" + sap.addCarets() + ")";

if (runtime) {

// We need to test that the expression fails at runtime.

// Ironically, that means that it must succeed at prepare time.

SqlValidator validator = factory.createValidator();

SqlNode n = parseAndValidate(validator, sql);

assertNotNull(n);

tester.checkFails(factory, sap, expectedError, runtime);

} else {

checkQueryFails(StringAndPos.of(sql),

expectedError);

}

}

which means it checks for failure only if it is not runtime...
so it could be any expected message and any number under cast and it continues passing...

Or did I do something wrong?

@snuyanzin, I think it should fail as you say, there seems to be something wrong.

I also tried changing runtime to false just to see, in this way it always fails (with a cast with overflow, and cast without, in both cases parsing succeeds, so the check fail throws an error), I guess there is something broken in the test auxiliary methods we rely on here.

The source of the problem lies outside the present PR, but it's nonetheless a blocker for merging (if confirmed), as we can't rely on the unit tests.

Maybe @mihaibudiu has more familiarity with this part of the code, and can shed some light.

I was expecting these tests to exercise the new code added to RexToLixTranslator, but this doesn't seem to be the case (a breakpoint there isn't hit when debugging such tests).

If that's a wrong assumption, what tests are covering the new code added to RexToLixTranslator, and what part of the code is specifically tested by above tests?

If you call the test from SqlOperatorTest it doesn't involve the evaluation.
You have to call the test from CalciteSqlOperatorTest. That class extends SqlOperatorTest but uses a test fixture which also evaluates the expressions. In the IDE you can do this easiest if you paste the following in CalciteSqlOperatorTest:

@Test public void testOverflow() { super.testOverflow(); }

Thanks for the refresher Mihai, I always forget about this behaviour in some of our test classes.

I have verified the tests, they correctly fail if a non-overflowing value/expression is passed, so they do their job.

Was able to test, thanks for clarification

linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java

asolimando

After @snuyanzin's comment I feel we need to get to the bottom of the test issue before merging, therefore marking the PR as "request changes" until clarified

mihaibudiu · 2024-01-16T23:02:48Z

After @snuyanzin's comment I feel we need to get to the bottom of the test issue before merging, therefore marking the PR as "request changes" until clarified

I have made the fixes you suggested. The tests should be run from the class CalciteSqlOperatorTest to exercise the evaluator.

asolimando · 2024-01-17T10:41:05Z

After @snuyanzin's comment I feel we need to get to the bottom of the test issue before merging, therefore marking the PR as "request changes" until clarified

I have made the fixes you suggested. The tests should be run from the class CalciteSqlOperatorTest to exercise the evaluator.

Thanks Mihai, the changes and the patch LGTM, can you please address the Sonar violation regarding the repeated occurrence of the string ".* out of range"? (just define it once at the beginning of the test for better maintainability in case the error messages changes).

PS: please don't force-push while the review process is still on-going, it makes reviewers' life more difficult for not much gain, thanks!

snuyanzin · 2024-01-17T19:11:37Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+    // Casting a floating point value larger than the maximum allowed value.
+    // 1e60 is larger than the largest BIGINT value allowed.
+    f.checkFails("SELECT cast(1e60+30 as tinyint)",
+        ".* out of range", true);


nit:
since there is already existing org.apache.calcite.sql.test.SqlOperatorFixture#OUT_OF_RANGE_MESSAGE would it make sense to reuse it here?

My last commit does exactly that. It removes one sonarcloud complaint, as pointed out by @asolimando.

The other sonarcloud complaint is about a complex method which I didn't really write, but I slightly modified, so I didn't try to reduce the complexity. This is after all just a test method.

Yes, Sonar has false positives sometimes, it's fine to address only what's reasonable (I consider fixing the complexity of an existing test method as as nice-to-have and totally optional), but most of the time it provides good hints for keeping the code quality high.

sonarcloud · 2024-01-18T01:32:20Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
97.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

asolimando

LGTM, thank you, Mihai!

…check overflow Signed-off-by: Mihai Budiu <mbudiu@feldera.com>

mihaibudiu force-pushed the issue6111 branch from 9d2cf51 to 617867f Compare November 17, 2023 18:34

zstan reviewed Nov 22, 2023

View reviewed changes

mihaibudiu force-pushed the issue6111 branch from 617867f to aadcdcd Compare December 6, 2023 19:25

mihaibudiu mentioned this pull request Dec 6, 2023

[CALCITE-6071] RexCall should carry source position information for runtime error reporting #3506

Open

zstan reviewed Dec 29, 2023

View reviewed changes

mihaibudiu force-pushed the issue6111 branch from aadcdcd to 4325f1e Compare December 29, 2023 22:37

mihaibudiu force-pushed the issue6111 branch from 4325f1e to cd8e63a Compare January 15, 2024 23:26

asolimando reviewed Jan 16, 2024

View reviewed changes

snuyanzin reviewed Jan 16, 2024

View reviewed changes

asolimando reviewed Jan 16, 2024

View reviewed changes

linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java Show resolved Hide resolved

asolimando requested changes Jan 16, 2024

View reviewed changes

mihaibudiu force-pushed the issue6111 branch from cd8e63a to 68398bf Compare January 16, 2024 23:01

mihaibudiu force-pushed the issue6111 branch from 68398bf to b59166f Compare January 16, 2024 23:22

snuyanzin reviewed Jan 17, 2024

View reviewed changes

mihaibudiu force-pushed the issue6111 branch from 6731e11 to 715a4b9 Compare January 18, 2024 01:06

asolimando reviewed Jan 18, 2024

View reviewed changes

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java Show resolved Hide resolved

asolimando approved these changes Jan 19, 2024

View reviewed changes

[CALCITE-6111] Explicit cast from expression to numeric type doesn't …

40b3bcc

…check overflow Signed-off-by: Mihai Budiu <mbudiu@feldera.com>

mihaibudiu force-pushed the issue6111 branch from 715a4b9 to 40b3bcc Compare January 19, 2024 22:24

mihaibudiu merged commit e2c84a6 into apache:main Jan 19, 2024
15 of 16 checks passed

mihaibudiu deleted the issue6111 branch January 19, 2024 23:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow #3522

[CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow #3522

mihaibudiu commented Nov 14, 2023

mihaibudiu commented Nov 14, 2023

zstan commented Nov 18, 2023

mihaibudiu commented Nov 19, 2023

zstan Nov 22, 2023

mihaibudiu Nov 22, 2023

mihaibudiu Nov 22, 2023

mihaibudiu commented Dec 27, 2023

zstan Dec 29, 2023

zstan Dec 29, 2023

mihaibudiu Dec 29, 2023

mihaibudiu commented Dec 29, 2023

mihaibudiu commented Jan 11, 2024

zstan commented Jan 14, 2024

asolimando left a comment

asolimando Jan 16, 2024

snuyanzin Jan 16, 2024

asolimando Jan 16, 2024 •

edited

asolimando Jan 16, 2024

mihaibudiu Jan 16, 2024

asolimando Jan 17, 2024

snuyanzin Jan 17, 2024

asolimando left a comment

mihaibudiu commented Jan 16, 2024

asolimando commented Jan 17, 2024

snuyanzin Jan 17, 2024

mihaibudiu Jan 18, 2024

asolimando Jan 18, 2024

sonarcloud bot commented Jan 18, 2024

asolimando left a comment

	f.checkFails("SELECT cast(100+30 as tinyint)",
	f.checkFails(String.format("SELECT cast(%d+30 as tinyint)", Byte.MAX_VALUE),

		f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)", Byte.MAX_VALUE),
		".* out of range", true);

	@Override public void checkFails(StringAndPos sap, String expectedError,
	boolean runtime) {
	final String sql = "values (" + sap.addCarets() + ")";
	if (runtime) {
	// We need to test that the expression fails at runtime.
	// Ironically, that means that it must succeed at prepare time.
	SqlValidator validator = factory.createValidator();
	SqlNode n = parseAndValidate(validator, sql);
	assertNotNull(n);
	tester.checkFails(factory, sap, expectedError, runtime);
	} else {
	checkQueryFails(StringAndPos.of(sql),
	expectedError);
	}
	}

[CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow #3522

[CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow #3522

Conversation

mihaibudiu commented Nov 14, 2023

mihaibudiu commented Nov 14, 2023

zstan commented Nov 18, 2023

mihaibudiu commented Nov 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaibudiu commented Dec 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaibudiu commented Dec 29, 2023

mihaibudiu commented Jan 11, 2024

zstan commented Jan 14, 2024

asolimando left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asolimando Jan 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asolimando left a comment

Choose a reason for hiding this comment

mihaibudiu commented Jan 16, 2024

asolimando commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 18, 2024

Quality Gate passed

asolimando left a comment

Choose a reason for hiding this comment

asolimando Jan 16, 2024 •

edited