You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/09/29 02:22:42 UTC
[GitHub] [calcite] xuyangzhong opened a new pull request #2555: [CALCITE-4777]Casting from DECIMAL to BOOLEAN throws an exception
xuyangzhong opened a new pull request #2555:
URL: https://github.com/apache/calcite/pull/2555
According to the discussion in [calcite-4777], casting from decimal to boolean is invalid, and an exception like "Cast function cannot convert value of type DECIMAL(2,1) to type BOOLEAN" will be thrown in the period of validating.
This pr changes:
- remove the coerce rule from decimal to boolean, which will lead to throw a conversion exception and let users know this is an invalid conversion.
- add some test cases in SqlOperatorBaseTest.
- change the reference.md to say this invalid conversion
--
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
[GitHub] [calcite] xuyangzhong commented on a change in pull request #2555: [CALCITE-4777]Casting from DECIMAL to BOOLEAN throws an exception
Posted by GitBox <gi...@apache.org>.
xuyangzhong commented on a change in pull request #2555:
URL: https://github.com/apache/calcite/pull/2555#discussion_r718996757
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
##########
@@ -1595,6 +1598,17 @@ protected static Calendar getCalendarNotTooNear(int timeUnit) {
tester.checkFails(
"cast(cast('blah' as varchar(10)) as boolean)", INVALID_CHAR_MESSAGE,
true);
+
+ tester.checkBoolean("cast(0 as boolean)", false);
Review comment:
got 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
[GitHub] [calcite] rubenada commented on a change in pull request #2555: [CALCITE-4777]Casting from DECIMAL to BOOLEAN throws an exception
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2555:
URL: https://github.com/apache/calcite/pull/2555#discussion_r719125001
##########
File path: core/src/main/java/org/apache/calcite/sql/type/SqlTypeCoercionRule.java
##########
@@ -202,7 +202,8 @@ private SqlTypeCoercionRule(Map<SqlTypeName, ImmutableSet<SqlTypeName>> map) {
coerceRules.copyValues(SqlTypeName.BOOLEAN)
.add(SqlTypeName.CHAR)
.add(SqlTypeName.VARCHAR)
- .addAll(SqlTypeName.NUMERIC_TYPES)
+ .addAll(SqlTypeName.INT_TYPES)
+ .addAll(SqlTypeName.APPROX_TYPES)
Review comment:
IMO it makes sense, if we say that `DECIMAL` cannot be cast into `BOOLEAN`; consider the same for `FLOAT`, `REAL` and `DOUBLE`, I think it is a consistent approach.
However, if we confirm to go in this direction:
- It seems some test will need adjustments
- The cast matrix in `reference.md` will need to be modified for those rows too.
--
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
[GitHub] [calcite] rubenada commented on pull request #2555: [CALCITE-4777]Casting from DECIMAL to BOOLEAN throws an exception
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2555:
URL: https://github.com/apache/calcite/pull/2555#issuecomment-937513773
@NobiGo my understanding was that casting decimal (and float, real, double) into boolean will be forbidden (implemented by this PR); but casting integer types into boolean is (and will continue being) allowed.
A different issue is that currently casting integer into boolean always returns false (instead of zero->false; non-zero->true), but there was another ticket to deal with this problem: CALCITE-4782.
Let us clarify this in the Jira ticket.
--
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
[GitHub] [calcite] xuyangzhong commented on pull request #2555: [CALCITE-4777]Casting from DECIMAL to BOOLEAN throws an exception
Posted by GitBox <gi...@apache.org>.
xuyangzhong commented on pull request #2555:
URL: https://github.com/apache/calcite/pull/2555#issuecomment-938273803
@rubenada done
--
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
[GitHub] [calcite] rubenada commented on pull request #2555: [CALCITE-4777]Casting from DECIMAL to BOOLEAN throws an exception
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2555:
URL: https://github.com/apache/calcite/pull/2555#issuecomment-937513773
@NobiGo my understanding was that casting decimal (and float, real, double) into boolean will be forbidden (implemented by this PR); but casting integer types into boolean is (and will continue being) allowed.
A different issue is that currently casting integer into boolean always returns false (instead of zero->false; non-zero->true), but there was another ticket to deal with this problem: CALCITE-4782.
Let us clarify this in the Jira ticket.
--
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
[GitHub] [calcite] rubenada commented on a change in pull request #2555: [CALCITE-4777]Casting from DECIMAL to BOOLEAN throws an exception
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2555:
URL: https://github.com/apache/calcite/pull/2555#discussion_r718695171
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
##########
@@ -1595,6 +1598,17 @@ protected static Calendar getCalendarNotTooNear(int timeUnit) {
tester.checkFails(
"cast(cast('blah' as varchar(10)) as boolean)", INVALID_CHAR_MESSAGE,
true);
+
+ tester.checkBoolean("cast(0 as boolean)", false);
Review comment:
For consistency, it should be `Boolean.FALSE` instead of `false`
--
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
[GitHub] [calcite] rubenada commented on a change in pull request #2555: [CALCITE-4777]Casting from DECIMAL to BOOLEAN throws an exception
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2555:
URL: https://github.com/apache/calcite/pull/2555#discussion_r718695171
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
##########
@@ -1595,6 +1598,17 @@ protected static Calendar getCalendarNotTooNear(int timeUnit) {
tester.checkFails(
"cast(cast('blah' as varchar(10)) as boolean)", INVALID_CHAR_MESSAGE,
true);
+
+ tester.checkBoolean("cast(0 as boolean)", false);
Review comment:
For consistency, it should be `Boolean.FALSE` instead of `false`
##########
File path: core/src/main/java/org/apache/calcite/sql/type/SqlTypeCoercionRule.java
##########
@@ -202,7 +202,8 @@ private SqlTypeCoercionRule(Map<SqlTypeName, ImmutableSet<SqlTypeName>> map) {
coerceRules.copyValues(SqlTypeName.BOOLEAN)
.add(SqlTypeName.CHAR)
.add(SqlTypeName.VARCHAR)
- .addAll(SqlTypeName.NUMERIC_TYPES)
+ .addAll(SqlTypeName.INT_TYPES)
+ .addAll(SqlTypeName.APPROX_TYPES)
Review comment:
`SqlTypeName.APPROX_TYPES` include `FLOAT`, `REAL` and `DOUBLE`. Are we sure casting these types into boolean always work?
It's strange, it seems that `SELECT CAST(CAST(0 AS float) AS BOOLEAN)` works fine (returns `false`). But if we try a more "complex" query: `select cast(cast(p0 as float) as boolean) from (values (0)) as t(p0)` ; it will fail at runtime with `CompileException: Line 21, Column 31: Cannot cast "double" to "boolean"`.
This type of query will be generated if we add these tests into `SqlOperatorBaseTest#testCastToBoolean`:
```
tester.checkBoolean("cast(cast(0 as float) as boolean)", Boolean.FALSE);
tester.checkBoolean("cast(cast(0 as real) as boolean)", Boolean.FALSE);
tester.checkBoolean("cast(cast(0 as double) as boolean)", Boolean.FALSE);
```
which, if I am not mistaken, will fail if we add them to `SqlOperatorBaseTest#testCastToBoolean`. Therefore, perhaps we should consider `FLOAT`, `REAL` and `DOUBLE` (together with `DECIMAL`) as non-castable into boolean?
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
##########
@@ -1595,6 +1598,17 @@ protected static Calendar getCalendarNotTooNear(int timeUnit) {
tester.checkFails(
"cast(cast('blah' as varchar(10)) as boolean)", INVALID_CHAR_MESSAGE,
true);
+
+ tester.checkBoolean("cast(0 as boolean)", false);
+
Review comment:
Perhaps we could add also the test for the "other int types"?
```
tester.checkBoolean("cast(cast(0 as tinyint) as boolean)", Boolean.FALSE);
tester.checkBoolean("cast(cast(0 as smallint) as boolean)", Boolean.FALSE);
tester.checkBoolean("cast(cast(0 as bigint) as boolean)", Boolean.FALSE);
```
--
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
[GitHub] [calcite] rubenada commented on a change in pull request #2555: [CALCITE-4777]Casting from DECIMAL to BOOLEAN throws an exception
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2555:
URL: https://github.com/apache/calcite/pull/2555#discussion_r718704717
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
##########
@@ -1595,6 +1598,17 @@ protected static Calendar getCalendarNotTooNear(int timeUnit) {
tester.checkFails(
"cast(cast('blah' as varchar(10)) as boolean)", INVALID_CHAR_MESSAGE,
true);
+
+ tester.checkBoolean("cast(0 as boolean)", false);
+
Review comment:
Perhaps we could add also the test for the "other int types"?
```
tester.checkBoolean("cast(cast(0 as tinyint) as boolean)", Boolean.FALSE);
tester.checkBoolean("cast(cast(0 as smallint) as boolean)", Boolean.FALSE);
tester.checkBoolean("cast(cast(0 as bigint) as boolean)", Boolean.FALSE);
```
--
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
[GitHub] [calcite] xuyangzhong commented on a change in pull request #2555: [CALCITE-4777]Casting from DECIMAL to BOOLEAN throws an exception
Posted by GitBox <gi...@apache.org>.
xuyangzhong commented on a change in pull request #2555:
URL: https://github.com/apache/calcite/pull/2555#discussion_r719010727
##########
File path: core/src/main/java/org/apache/calcite/sql/type/SqlTypeCoercionRule.java
##########
@@ -202,7 +202,8 @@ private SqlTypeCoercionRule(Map<SqlTypeName, ImmutableSet<SqlTypeName>> map) {
coerceRules.copyValues(SqlTypeName.BOOLEAN)
.add(SqlTypeName.CHAR)
.add(SqlTypeName.VARCHAR)
- .addAll(SqlTypeName.NUMERIC_TYPES)
+ .addAll(SqlTypeName.INT_TYPES)
+ .addAll(SqlTypeName.APPROX_TYPES)
Review comment:
About this bug I create a JIRA yesterday(in https://issues.apache.org/jira/browse/CALCITE-4810) before this pr.
I tested the sql "tester.checkBoolean("cast(cast(0.0 as double) as boolean)", Boolean.FALSE);" in master where i didn't apply my change and it threw the same exception just like you says.
I tried to find the reason and add the message into [CALCITE-4810]. It seems the bug that these two behavoirs codegen and constant simplifying is not unified.
I think your suggestion that avoids casting from float, real, double and decimal to boolean is reasonable and this also can solve this problem of inconsistent behavior. And more importantly, it follows the SQL standard.
--
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
[GitHub] [calcite] NobiGo commented on pull request #2555: [CALCITE-4777]Casting from DECIMAL to BOOLEAN throws an exception
Posted by GitBox <gi...@apache.org>.
NobiGo commented on pull request #2555:
URL: https://github.com/apache/calcite/pull/2555#issuecomment-937346994
> LGTM @xuyangzhong the PR looks in a good shape, could you please squash commits into a single one so that we can merge it shortly?
@rubenada As discussed in Jira（According to my understanding of the comments）. The SQL:
```
select cast(1 as boolean)
select cast(0 as boolean)
```
is also invalid.
--
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
[GitHub] [calcite] rubenada commented on a change in pull request #2555: [CALCITE-4777]Casting from DECIMAL to BOOLEAN throws an exception
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2555:
URL: https://github.com/apache/calcite/pull/2555#discussion_r718702589
##########
File path: core/src/main/java/org/apache/calcite/sql/type/SqlTypeCoercionRule.java
##########
@@ -202,7 +202,8 @@ private SqlTypeCoercionRule(Map<SqlTypeName, ImmutableSet<SqlTypeName>> map) {
coerceRules.copyValues(SqlTypeName.BOOLEAN)
.add(SqlTypeName.CHAR)
.add(SqlTypeName.VARCHAR)
- .addAll(SqlTypeName.NUMERIC_TYPES)
+ .addAll(SqlTypeName.INT_TYPES)
+ .addAll(SqlTypeName.APPROX_TYPES)
Review comment:
`SqlTypeName.APPROX_TYPES` include `FLOAT`, `REAL` and `DOUBLE`. Are we sure casting these types into boolean always work?
It's strange, it seems that `SELECT CAST(CAST(0 AS float) AS BOOLEAN)` works fine (returns `false`). But if we try a more "complex" query: `select cast(cast(p0 as float) as boolean) from (values (0)) as t(p0)` ; it will fail at runtime with `CompileException: Line 21, Column 31: Cannot cast "double" to "boolean"`.
This type of query will be generated if we add these tests into `SqlOperatorBaseTest#testCastToBoolean`:
```
tester.checkBoolean("cast(cast(0 as float) as boolean)", Boolean.FALSE);
tester.checkBoolean("cast(cast(0 as real) as boolean)", Boolean.FALSE);
tester.checkBoolean("cast(cast(0 as double) as boolean)", Boolean.FALSE);
```
which, if I am not mistaken, will fail if we add them to `SqlOperatorBaseTest#testCastToBoolean`. Therefore, perhaps we should consider `FLOAT`, `REAL` and `DOUBLE` (together with `DECIMAL`) as non-castable into boolean?
--
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
[GitHub] [calcite] xuyangzhong commented on a change in pull request #2555: [CALCITE-4777]Casting from DECIMAL to BOOLEAN throws an exception
Posted by GitBox <gi...@apache.org>.
xuyangzhong commented on a change in pull request #2555:
URL: https://github.com/apache/calcite/pull/2555#discussion_r719010777
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
##########
@@ -1595,6 +1598,17 @@ protected static Calendar getCalendarNotTooNear(int timeUnit) {
tester.checkFails(
"cast(cast('blah' as varchar(10)) as boolean)", INVALID_CHAR_MESSAGE,
true);
+
+ tester.checkBoolean("cast(0 as boolean)", false);
+
Review comment:
got 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
[GitHub] [calcite] rubenada commented on pull request #2555: [CALCITE-4777]Casting from DECIMAL to BOOLEAN throws an exception
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2555:
URL: https://github.com/apache/calcite/pull/2555#issuecomment-935644628
LGTM
@xuyangzhong the PR looks in a good shape, could you please squash commits into a single one so that we can merge it shortly?
--
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
[GitHub] [calcite] NobiGo commented on pull request #2555: [CALCITE-4777]Casting from DECIMAL to BOOLEAN throws an exception
Posted by GitBox <gi...@apache.org>.
NobiGo commented on pull request #2555:
URL: https://github.com/apache/calcite/pull/2555#issuecomment-937346994
> LGTM @xuyangzhong the PR looks in a good shape, could you please squash commits into a single one so that we can merge it shortly?
@rubenada As discussed in Jira（According to my understanding of the comments）. The SQL:
```
select cast(1 as boolean)
select cast(0 as boolean)
```
is also invalid.
--
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