You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "mihaibudiu (via GitHub)" <gi...@apache.org> on 2023/11/14 04:51:37 UTC
[PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
mihaibudiu opened a new pull request, #3522:
URL: https://github.com/apache/calcite/pull/3522
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on PR #3522:
URL: https://github.com/apache/calcite/pull/3522#issuecomment-1890997298
@mihaibudiu honestly, overall looks good but as for me this issue need additional eyes here, can you found one more reviewer on dev list ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1402530561
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13412,6 +13414,34 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6111">[CALCITE-6111]
+ * 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)",
+ ".* out of range", true);
+ f.checkFails("SELECT cast(100000+30 as smallint)",
+ ".* out of range", true);
+ f.checkFails("SELECT cast(10000000000+30 as int)",
+ ".* out of range", true);
+
+ f.checkFails("SELECT cast(1e60+30 as tinyint)",
+ ".* out of range", true);
+ f.checkFails("SELECT cast(1e60+30 as smallint)",
+ ".* out of range", true);
+ f.checkFails("SELECT cast(1e60+30 as int)",
+ ".* out of range", true);
+ f.checkFails("SELECT cast(1e60+30 as bigint)",
Review Comment:
These cases should be covered by the test `testCastExactNumericLimits`, where I re-enabled some of the tests that were disabled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1453272159
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13695,6 +13697,43 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6111">[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow</a>. */
+ @Test public void testOverflow() {
+ final SqlOperatorFixture f = fixture();
+ f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)", Byte.MAX_VALUE),
+ ".* out of range", true);
Review Comment:
@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), 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1438065097
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13412,6 +13414,34 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6111">[CALCITE-6111]
+ * 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)",
Review Comment:
what about implicit casts, is it all ok there ? or it out of scope ?
SELECT 9223372036854775807 + 1;
SELECT 2147483647 + 1;
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1453291779
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13695,6 +13697,43 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6111">[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow</a>. */
+ @Test public void testOverflow() {
+ final SqlOperatorFixture f = fixture();
+ f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)", Byte.MAX_VALUE),
+ ".* out of range", true);
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3522:
URL: https://github.com/apache/calcite/pull/3522#issuecomment-1897612999
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3522) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3522&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3522&resolved=false&inNewCodePeriod=true)
[97.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3522&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3522&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3522)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1456674069
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13695,6 +13698,43 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6111">[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow</a>. */
+ @Test public void testOverflow() {
+ final SqlOperatorFixture f = fixture();
+ f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)", Byte.MAX_VALUE),
+ ".* out of range", true);
+ f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as smallint)", Short.MAX_VALUE),
+ ".* out of range", true);
+ // We use a long value because otherwise calcite interprets the literal as an integer.
+ f.checkFails(String.format(Locale.US, "SELECT cast(%d as int)", Long.MAX_VALUE),
+ ".* out of range", true);
+
+ // 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);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3522:
URL: https://github.com/apache/calcite/pull/3522#issuecomment-1894706587
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3522) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3522&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3522&resolved=false&inNewCodePeriod=true)
[97.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3522&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3522&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3522)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3522:
URL: https://github.com/apache/calcite/pull/3522#issuecomment-1843663845
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3522)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3522&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3522&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3522&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3522&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3522&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3522&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3522&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3522&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3522&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3522&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3522&resolved=false&types=CODE_SMELL) [3 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3522&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3522&metric=new_coverage&view=list) [97.6% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3522&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3522&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3522&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1453272159
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13695,6 +13697,43 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6111">[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow</a>. */
+ @Test public void testOverflow() {
+ final SqlOperatorFixture f = fixture();
+ f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)", Byte.MAX_VALUE),
+ ".* out of range", true);
Review Comment:
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3522:
URL: https://github.com/apache/calcite/pull/3522#issuecomment-1817938772
These tests are very useful. They cover many more cases than this PR handles. Here we only do casts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1455199716
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13695,6 +13697,43 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6111">[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow</a>. */
+ @Test public void testOverflow() {
+ final SqlOperatorFixture f = fixture();
+ f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)", Byte.MAX_VALUE),
+ ".* out of range", true);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1401520303
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13412,6 +13414,34 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6111">[CALCITE-6111]
+ * 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)",
+ ".* out of range", true);
+ f.checkFails("SELECT cast(100000+30 as smallint)",
+ ".* out of range", true);
+ f.checkFails("SELECT cast(10000000000+30 as int)",
+ ".* out of range", true);
+
+ f.checkFails("SELECT cast(1e60+30 as tinyint)",
+ ".* out of range", true);
+ f.checkFails("SELECT cast(1e60+30 as smallint)",
+ ".* out of range", true);
+ f.checkFails("SELECT cast(1e60+30 as int)",
+ ".* out of range", true);
+ f.checkFails("SELECT cast(1e60+30 as bigint)",
Review Comment:
what about: SELECT CAST(2147483648 as BIGINT) ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1454153342
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -657,10 +661,8 @@ void testCastExactNumericLimits(CastType castType, SqlOperatorFixture f) {
f.checkCastToString(numeric.minNumericString, null, null, castType);
f.checkCastToString(numeric.minNumericString, type, null, castType);
- if (Bug.CALCITE_2539_FIXED) {
- f.checkCastFails("'notnumeric'", type, INVALID_CHAR_MESSAGE, true,
- castType);
- }
+ f.checkCastFails("'notnumeric'", type, INVALID_CHAR_MESSAGE, true,
Review Comment:
Yes, CALCITE-2539 seems to cover multiple issues, and I have fixed only one of them here. There are other disabled tests guarded by this Bug.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3522:
URL: https://github.com/apache/calcite/pull/3522#issuecomment-1894659736
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3522:
URL: https://github.com/apache/calcite/pull/3522#issuecomment-1810698360
The CI failure is from the Druid tests, so I guess this PR is ready for review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1454152204
##########
linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java:
##########
@@ -384,6 +384,10 @@ static void checkRoundedRange(Number value, double min, double max) {
}
}
+ public static @Nullable Object integerCast(Primitive primitive, final Object value) {
Review Comment:
Not quite reflection. The way Calcite evaluates constant expressions is to generate a Java class with a Java method which is then compiled, loaded and executed at runtime.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on PR #3522:
URL: https://github.com/apache/calcite/pull/3522#issuecomment-1895541892
> > 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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1438063133
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13412,6 +13414,34 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6111">[CALCITE-6111]
+ * 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)",
Review Comment:
as for me - it`s hard to parse such a tests.
```suggestion
f.checkFails(String.format("SELECT cast(%d+30 as tinyint)", Byte.MAX_VALUE),
```
and the same for all near
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1453252536
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13695,6 +13697,43 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6111">[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow</a>. */
+ @Test public void testOverflow() {
+ final SqlOperatorFixture f = fixture();
+ f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)", Byte.MAX_VALUE),
+ ".* out of range", true);
Review Comment:
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 https://github.com/apache/calcite/blob/0bec957071468a2e54a22519290ac101a752fcad/testkit/src/main/java/org/apache/calcite/test/SqlOperatorFixtureImpl.java#L157-L171
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1458498638
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -643,11 +647,12 @@ void testCastExactNumericLimits(CastType castType, SqlOperatorFixture f) {
f.checkCastToScalarOkay("'" + numeric.minNumericString + "'",
type, numeric.minNumericString, castType);
- if (Bug.CALCITE_2539_FIXED) {
+ if (numeric != Numeric.DECIMAL5_2) {
+ // The above condition is for bug CALCITE-6078
f.checkCastFails("'" + numeric.maxOverflowNumericString + "'",
- type, OUT_OF_RANGE_MESSAGE, true, castType);
+ type, WRONG_FORMAT_MESSAGE, true, castType);
Review Comment:
You are right, I missed the extra quote, sorry about that!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3522:
URL: https://github.com/apache/calcite/pull/3522#issuecomment-1888038624
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1458056660
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -643,11 +647,12 @@ void testCastExactNumericLimits(CastType castType, SqlOperatorFixture f) {
f.checkCastToScalarOkay("'" + numeric.minNumericString + "'",
type, numeric.minNumericString, castType);
- if (Bug.CALCITE_2539_FIXED) {
+ if (numeric != Numeric.DECIMAL5_2) {
+ // The above condition is for bug CALCITE-6078
f.checkCastFails("'" + numeric.maxOverflowNumericString + "'",
- type, OUT_OF_RANGE_MESSAGE, true, castType);
+ type, WRONG_FORMAT_MESSAGE, true, castType);
Review Comment:
Actually this variant of the test invokes something like `Integer.parseInt(string)`. So this is in fact a cast from a string to integer -- notice the single quotes added around the literal -- which gives a different error than a cast from a larger integer to an integer. The former produces a WRONG_FORMAT, the latter gives OUT_OF_RANGE. We could in principle trap the exception and rewrite the message, but it *is* a different exception in Java in the simplification code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1456343324
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13695,6 +13698,43 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6111">[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow</a>. */
+ @Test public void testOverflow() {
+ final SqlOperatorFixture f = fixture();
+ f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)", Byte.MAX_VALUE),
+ ".* out of range", true);
+ f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as smallint)", Short.MAX_VALUE),
+ ".* out of range", true);
+ // We use a long value because otherwise calcite interprets the literal as an integer.
+ f.checkFails(String.format(Locale.US, "SELECT cast(%d as int)", Long.MAX_VALUE),
+ ".* out of range", true);
+
+ // 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);
Review Comment:
nit:
since there is already existing `org.apache.calcite.sql.test.SqlOperatorFixture#OUT_OF_RANGE_MESSAGE` would it make sense to reuse it here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1456341289
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13695,6 +13697,43 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6111">[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow</a>. */
+ @Test public void testOverflow() {
+ final SqlOperatorFixture f = fixture();
+ f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)", Byte.MAX_VALUE),
+ ".* out of range", true);
Review Comment:
Was able to test, thanks for clarification
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1453283126
##########
linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java:
##########
@@ -384,6 +384,10 @@ static void checkRoundedRange(Number value, double min, double max) {
}
}
+ public static @Nullable Object integerCast(Primitive primitive, final Object value) {
Review Comment:
Q: Is this used via reflection? My IDE suggests that the method is unused.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1457090549
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -643,11 +647,12 @@ void testCastExactNumericLimits(CastType castType, SqlOperatorFixture f) {
f.checkCastToScalarOkay("'" + numeric.minNumericString + "'",
type, numeric.minNumericString, castType);
- if (Bug.CALCITE_2539_FIXED) {
+ if (numeric != Numeric.DECIMAL5_2) {
+ // The above condition is for bug CALCITE-6078
f.checkCastFails("'" + numeric.maxOverflowNumericString + "'",
- type, OUT_OF_RANGE_MESSAGE, true, castType);
+ type, WRONG_FORMAT_MESSAGE, true, castType);
Review Comment:
From the variable name, it seems that `OUT_OF_RANGE_MESSAGE` was the correct error we expected (similarly to the checks in the `if` statement starting at line 635)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1458056857
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -643,11 +647,12 @@ void testCastExactNumericLimits(CastType castType, SqlOperatorFixture f) {
f.checkCastToScalarOkay("'" + numeric.minNumericString + "'",
type, numeric.minNumericString, castType);
- if (Bug.CALCITE_2539_FIXED) {
+ if (numeric != Numeric.DECIMAL5_2) {
+ // The above condition is for bug CALCITE-6078
f.checkCastFails("'" + numeric.maxOverflowNumericString + "'",
- type, OUT_OF_RANGE_MESSAGE, true, castType);
+ type, WRONG_FORMAT_MESSAGE, true, castType);
Review Comment:
The integer-to-integer cast is tested a few lines above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #3522:
URL: https://github.com/apache/calcite/pull/3522
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1438362117
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13412,6 +13414,34 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6111">[CALCITE-6111]
+ * 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)",
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3522:
URL: https://github.com/apache/calcite/pull/3522#issuecomment-1870541231
This is a bugfix PR, can someone please review it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on PR #3522:
URL: https://github.com/apache/calcite/pull/3522#issuecomment-1817496932
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1402555661
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13412,6 +13414,34 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6111">[CALCITE-6111]
+ * 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)",
+ ".* out of range", true);
+ f.checkFails("SELECT cast(100000+30 as smallint)",
+ ".* out of range", true);
+ f.checkFails("SELECT cast(10000000000+30 as int)",
+ ".* out of range", true);
+
+ f.checkFails("SELECT cast(1e60+30 as tinyint)",
+ ".* out of range", true);
+ f.checkFails("SELECT cast(1e60+30 as smallint)",
+ ".* out of range", true);
+ f.checkFails("SELECT cast(1e60+30 as int)",
+ ".* out of range", true);
+ f.checkFails("SELECT cast(1e60+30 as bigint)",
Review Comment:
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)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3522:
URL: https://github.com/apache/calcite/pull/3522#issuecomment-1872382983
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3522) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[4 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3522&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3522&resolved=false&inNewCodePeriod=true)
[97.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3522&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3522&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3522)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3522:
URL: https://github.com/apache/calcite/pull/3522#issuecomment-1872375361
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1453189554
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -643,11 +646,12 @@ void testCastExactNumericLimits(CastType castType, SqlOperatorFixture f) {
f.checkCastToScalarOkay("'" + numeric.minNumericString + "'",
type, numeric.minNumericString, castType);
- if (Bug.CALCITE_2539_FIXED) {
+ if (numeric != Numeric.DECIMAL5_2) {
+ // The above conditoin is for bug CALCITE-6078
f.checkCastFails("'" + numeric.maxOverflowNumericString + "'",
- type, OUT_OF_RANGE_MESSAGE, true, castType);
+ type, "Number has wrong format.*", true, castType);
Review Comment:
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:
##########
@@ -643,11 +646,12 @@ void testCastExactNumericLimits(CastType castType, SqlOperatorFixture f) {
f.checkCastToScalarOkay("'" + numeric.minNumericString + "'",
type, numeric.minNumericString, castType);
- if (Bug.CALCITE_2539_FIXED) {
+ if (numeric != Numeric.DECIMAL5_2) {
+ // The above conditoin is for bug CALCITE-6078
Review Comment:
```suggestion
// This condition is for bug [CALCITE-6078], not yet fixed
```
fixing a typo, reusing the same comment as above
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -643,11 +646,12 @@ void testCastExactNumericLimits(CastType castType, SqlOperatorFixture f) {
f.checkCastToScalarOkay("'" + numeric.minNumericString + "'",
type, numeric.minNumericString, castType);
- if (Bug.CALCITE_2539_FIXED) {
+ if (numeric != Numeric.DECIMAL5_2) {
+ // The above conditoin is for bug CALCITE-6078
f.checkCastFails("'" + numeric.maxOverflowNumericString + "'",
- type, OUT_OF_RANGE_MESSAGE, true, castType);
+ type, "Number has wrong format.*", true, castType);
f.checkCastFails("'" + numeric.minOverflowNumericString + "'",
- type, OUT_OF_RANGE_MESSAGE, true, castType);
+ type, "Number has wrong format.*", true, castType);
Review Comment:
fix here too if the proposal above is retained
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -657,10 +661,8 @@ void testCastExactNumericLimits(CastType castType, SqlOperatorFixture f) {
f.checkCastToString(numeric.minNumericString, null, null, castType);
f.checkCastToString(numeric.minNumericString, type, null, castType);
- if (Bug.CALCITE_2539_FIXED) {
- f.checkCastFails("'notnumeric'", type, INVALID_CHAR_MESSAGE, true,
- castType);
- }
+ f.checkCastFails("'notnumeric'", type, INVALID_CHAR_MESSAGE, true,
Review Comment:
Question: `CALCITE-2539` isn't currently fixed, I guess you just verified that the test passes and so confirmed it was safe to remove the "guard", right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1454159011
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13695,6 +13697,43 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6111">[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow</a>. */
+ @Test public void testOverflow() {
+ final SqlOperatorFixture f = fixture();
+ f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)", Byte.MAX_VALUE),
+ ".* out of range", true);
Review Comment:
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();
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1457093609
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -13695,6 +13698,43 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6111">[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow</a>. */
+ @Test public void testOverflow() {
+ final SqlOperatorFixture f = fixture();
+ f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)", Byte.MAX_VALUE),
+ ".* out of range", true);
+ f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as smallint)", Short.MAX_VALUE),
+ ".* out of range", true);
+ // We use a long value because otherwise calcite interprets the literal as an integer.
+ f.checkFails(String.format(Locale.US, "SELECT cast(%d as int)", Long.MAX_VALUE),
+ ".* out of range", true);
+
+ // 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);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3522:
URL: https://github.com/apache/calcite/pull/3522#issuecomment-1897581471
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3522) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3522&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3522&resolved=false&inNewCodePeriod=true)
[97.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3522&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3522&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3522)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org