You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "xuzifu666 (via GitHub)" <gi...@apache.org> on 2025/12/11 10:30:44 UTC
[PR] [CALCITE-7323] Result of cast Number to Boolean is not correct [calcite]
xuzifu666 opened a new pull request, #4679:
URL: https://github.com/apache/calcite/pull/4679
jira: https://issues.apache.org/jira/browse/CALCITE-7323
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4679:
URL: https://github.com/apache/calcite/pull/4679#issuecomment-3663096516
@xuzifu666 feel free to merge this and mark the JIRA issue as resolved
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2615967745
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -825,6 +825,18 @@ void testCastToExactNumeric(CastType castType, SqlOperatorFixture f) {
f.checkNull("CAST(CAST(NULL AS VARCHAR) AS VARBINARY)");
}
+ /**
+ * Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-7323">
+ * Result of cast Number to Boolean is not correct</a>. */
+ @Test public void testNumbericBooleanCast() {
+ SqlOperatorFixture f = fixture();
+ f.checkScalar("CAST(1 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(0 AS BOOLEAN)", "false", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(1.2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
Review Comment:
I checked in the ```SqlOperatorTest``` and it seems to only support non constant type tests. I just added a test and I'm not sure if this is possible.
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on PR #4679:
URL: https://github.com/apache/calcite/pull/4679#issuecomment-3658093161
> I think you can squash the commits.
Okay, 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
Re: [PR] [CALCITE-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2615994151
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -825,6 +825,18 @@ void testCastToExactNumeric(CastType castType, SqlOperatorFixture f) {
f.checkNull("CAST(CAST(NULL AS VARCHAR) AS VARBINARY)");
}
+ /**
+ * Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-7323">
+ * Result of cast Number to Boolean is not correct</a>. */
+ @Test public void testNumbericBooleanCast() {
+ SqlOperatorFixture f = fixture();
+ f.checkScalar("CAST(1 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(0 AS BOOLEAN)", "false", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(1.2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
Review Comment:
quidem tests are fine
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4679:
URL: https://github.com/apache/calcite/pull/4679#issuecomment-3648719926
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4679) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4679&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4679&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4679&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [89.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4679&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4679&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4679)
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4679:
URL: https://github.com/apache/calcite/pull/4679#issuecomment-3645407314
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4679) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4679&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4679&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4679&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [87.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4679&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4679&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4679)
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2613665578
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -825,6 +825,18 @@ void testCastToExactNumeric(CastType castType, SqlOperatorFixture f) {
f.checkNull("CAST(CAST(NULL AS VARCHAR) AS VARBINARY)");
}
+ /**
+ * Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-7323">
+ * Result of cast Number to Boolean is not correct</a>. */
+ @Test public void testNumbericBooleanCast() {
+ SqlOperatorFixture f = fixture();
+ f.checkScalar("CAST(1 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(0 AS BOOLEAN)", "false", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(1.2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
Review Comment:
I added some test cases related to NULL and Double, and the results should be as expected.
I added the variable type to RelToSqlConverterTest (which will convert it to var <>0), but I don't seem to fully understand the test way for ```-0e0```.
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -430,6 +430,9 @@ private Expression getConvertExpression(
return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
default:
+ if (sourceType.getFamily() == SqlTypeFamily.NUMERIC) {
+ return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
Review Comment:
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
Re: [PR] [CALCITE-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4679:
URL: https://github.com/apache/calcite/pull/4679#issuecomment-3645338012
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4679) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4679&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4679&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4679&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [87.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4679&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4679&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4679)
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4679:
URL: https://github.com/apache/calcite/pull/4679#issuecomment-3648778714
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4679) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4679&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4679&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4679&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [89.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4679&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4679&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4679)
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2613663290
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -430,6 +430,9 @@ private Expression getConvertExpression(
return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
default:
+ if (sourceType.getFamily() == SqlTypeFamily.NUMERIC) {
+ return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
Review Comment:
I added some test cases related to ```NULL``` and ```Double```, and the results should be as expected.
I added the variable type to ```RelToSqlConverterTest``` (which will convert it to var <>0), but I don't seem to fully understand the test way for -0e0. @mihaibudiu
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2613663290
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -430,6 +430,9 @@ private Expression getConvertExpression(
return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
default:
+ if (sourceType.getFamily() == SqlTypeFamily.NUMERIC) {
+ return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
Review Comment:
I added some test cases related to ```NULL``` and ```Double```, and the results should be as expected.
I added the variable type to ```RelToSqlConverterTest``` (which will convert it to var <>0), but I don't seem to fully understand the test way for -0e0. @mihaibudiu
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4679:
URL: https://github.com/apache/calcite/pull/4679#issuecomment-3657106104
I think you can squash the commits.
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2615972792
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -825,6 +825,18 @@ void testCastToExactNumeric(CastType castType, SqlOperatorFixture f) {
f.checkNull("CAST(CAST(NULL AS VARCHAR) AS VARBINARY)");
}
+ /**
+ * Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-7323">
+ * Result of cast Number to Boolean is not correct</a>. */
+ @Test public void testNumbericBooleanCast() {
+ SqlOperatorFixture f = fixture();
+ f.checkScalar("CAST(1 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(0 AS BOOLEAN)", "false", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(1.2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
Review Comment:
no one said that the test has to be in this file.
constant expressions are evaluated slightly differently than runtime expressions, that's why it would be nice to have a test with non-constant values 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
Re: [PR] [CALCITE-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on PR #4679:
URL: https://github.com/apache/calcite/pull/4679#issuecomment-3659062558
I would merge this pr in next 24h, if no other comments, thanks for review @mihaibudiu @xiedeyantu
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2612580892
##########
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##########
@@ -3152,7 +3152,9 @@ private SqlOperator getNoDeterministicOperator() {
checkSimplify(cast(literal(1), varcharType), "'1':VARCHAR(10)");
checkSimplifyUnchanged(cast(literalAbc, booleanType));
checkSimplify(cast(literal(1), booleanType),
- "false"); // different from Hive
+ "true"); // different from Hive
Review Comment:
Thanks, here not need this comment, I would delete it.
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -430,6 +430,9 @@ private Expression getConvertExpression(
return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
default:
+ if (sourceType.getFamily() == SqlTypeFamily.NUMERIC) {
Review Comment:
I initially did it this way too, but considering that there will be many ```SqlTypeFamily.NUMERIC``` types, if new subtypes of this type are added in the future, this part will also need to be modified accordingly, which may cause omissions. What do you think about this?
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -825,6 +825,18 @@ void testCastToExactNumeric(CastType castType, SqlOperatorFixture f) {
f.checkNull("CAST(CAST(NULL AS VARCHAR) AS VARBINARY)");
}
+ /**
+ * Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-7323">
+ * Result of cast Number to Boolean is not correct</a>. */
+ @Test public void testNumbericBooleanCast() {
+ SqlOperatorFixture f = fixture();
+ f.checkScalar("CAST(1 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(0 AS BOOLEAN)", "false", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(1.2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
Review Comment:
Thanks for reminder, I would add more case about it.
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -430,6 +430,9 @@ private Expression getConvertExpression(
return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
default:
+ if (sourceType.getFamily() == SqlTypeFamily.NUMERIC) {
+ return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
Review Comment:
I've confirmed that ```BuiltInMethod.STRING_TO_BOOLEAN``` directly calls ```SqlFunction.boolean toBoolean(Object o)```. It processes both String and Number objects; it just seems the name is a bit odd.
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2612587236
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -430,6 +430,9 @@ private Expression getConvertExpression(
return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
default:
+ if (sourceType.getFamily() == SqlTypeFamily.NUMERIC) {
+ return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
Review Comment:
This is a coincidence. There are other methods which may be a better fit:
```
4737: public static boolean toBoolean(Number number) {
```
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2610767007
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -430,6 +430,9 @@ private Expression getConvertExpression(
return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
default:
+ if (sourceType.getFamily() == SqlTypeFamily.NUMERIC) {
Review Comment:
Would it be better to keep the same case syntax as above?
##########
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##########
@@ -3152,7 +3152,9 @@ private SqlOperator getNoDeterministicOperator() {
checkSimplify(cast(literal(1), varcharType), "'1':VARCHAR(10)");
checkSimplifyUnchanged(cast(literalAbc, booleanType));
checkSimplify(cast(literal(1), booleanType),
- "false"); // different from Hive
+ "true"); // different from Hive
Review Comment:
Is this comment still needed?
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4679:
URL: https://github.com/apache/calcite/pull/4679#issuecomment-3658143210
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4679) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4679&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4679&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4679&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [89.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4679&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4679&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4679)
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2613650777
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -430,6 +430,9 @@ private Expression getConvertExpression(
return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
default:
+ if (sourceType.getFamily() == SqlTypeFamily.NUMERIC) {
Review Comment:
OK，your suggestion is reasonable, and I had changed as 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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2615993366
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -825,6 +825,18 @@ void testCastToExactNumeric(CastType castType, SqlOperatorFixture f) {
f.checkNull("CAST(CAST(NULL AS VARCHAR) AS VARBINARY)");
}
+ /**
+ * Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-7323">
+ * Result of cast Number to Boolean is not correct</a>. */
+ @Test public void testNumbericBooleanCast() {
+ SqlOperatorFixture f = fixture();
+ f.checkScalar("CAST(1 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(0 AS BOOLEAN)", "false", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(1.2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
Review Comment:
Yes, it is necessary，based on my understanding, I added a QuidemTest in ```cast.iq```, which can be used with non-constant values. Is it also OK?
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2615965977
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -825,6 +825,36 @@ void testCastToExactNumeric(CastType castType, SqlOperatorFixture f) {
f.checkNull("CAST(CAST(NULL AS VARCHAR) AS VARBINARY)");
}
+ /**
+ * Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-7323">
+ * Result of cast Number to Boolean is not correct</a>. */
+ @Test public void testNumbericBooleanCast() {
+ SqlOperatorFixture f = fixture();
+ f.setFor(SqlStdOperatorTable.MAX, VM_EXPAND);
+ String[] values = {"0", "CAST(null AS INTEGER)", "2", "2"};
+ f.checkAgg("cast(max(x) as BOOLEAN)", values, isSingle(true));
Review Comment:
It seems that ```SqlOperatorTest``` primarily contains tests for the constant type. I'm unsure whether this test pertains to the non-constant type. Additionally, I added a test for the var type in ```RelToSqlConverterTest```, but I'm not entirely certain if it complies. @mihaibudiu
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2611706526
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -430,6 +430,9 @@ private Expression getConvertExpression(
return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
default:
+ if (sourceType.getFamily() == SqlTypeFamily.NUMERIC) {
+ return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
Review Comment:
I don't understand why you call string_to_boolean for a numeric value
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -825,6 +825,18 @@ void testCastToExactNumeric(CastType castType, SqlOperatorFixture f) {
f.checkNull("CAST(CAST(NULL AS VARCHAR) AS VARBINARY)");
}
+ /**
+ * Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-7323">
+ * Result of cast Number to Boolean is not correct</a>. */
+ @Test public void testNumbericBooleanCast() {
+ SqlOperatorFixture f = fixture();
+ f.checkScalar("CAST(1 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(0 AS BOOLEAN)", "false", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(1.2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
Review Comment:
what happens for DOUBLE values?
Can you try with 0e0 and a value which evaluates to -0e0? (You cannot write -0e0 as a literal).
Can you add tests for non-constant values and NULL values?
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2612591198
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -430,6 +430,9 @@ private Expression getConvertExpression(
return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
default:
+ if (sourceType.getFamily() == SqlTypeFamily.NUMERIC) {
Review Comment:
I don't think this is a good approach because this style of writing cannot guarantee compatibility with future new types of content. The logic should be clear.
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2612599286
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -430,6 +430,9 @@ private Expression getConvertExpression(
return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
default:
+ if (sourceType.getFamily() == SqlTypeFamily.NUMERIC) {
+ return Expressions.call(BuiltInMethod.STRING_TO_BOOLEAN.method, operand);
Review Comment:
OK, it is better, I would change as this way.
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 merged PR #4679:
URL: https://github.com/apache/calcite/pull/4679
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4679:
URL: https://github.com/apache/calcite/pull/4679#issuecomment-3641360886
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4679) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4679&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4679&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4679&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4679&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4679&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4679)
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2615911431
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -825,6 +825,18 @@ void testCastToExactNumeric(CastType castType, SqlOperatorFixture f) {
f.checkNull("CAST(CAST(NULL AS VARCHAR) AS VARBINARY)");
}
+ /**
+ * Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-7323">
+ * Result of cast Number to Boolean is not correct</a>. */
+ @Test public void testNumbericBooleanCast() {
+ SqlOperatorFixture f = fixture();
+ f.checkScalar("CAST(1 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(0 AS BOOLEAN)", "false", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(1.2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
Review Comment:
I don't see any tests without constants, can you please add at least one?
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2615967745
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -825,6 +825,18 @@ void testCastToExactNumeric(CastType castType, SqlOperatorFixture f) {
f.checkNull("CAST(CAST(NULL AS VARCHAR) AS VARBINARY)");
}
+ /**
+ * Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-7323">
+ * Result of cast Number to Boolean is not correct</a>. */
+ @Test public void testNumbericBooleanCast() {
+ SqlOperatorFixture f = fixture();
+ f.checkScalar("CAST(1 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(0 AS BOOLEAN)", "false", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(1.2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
Review Comment:
I checked in the SQL ```OperatorTest``` and it seems to only support non constant type tests. I just added a test and I'm not sure if this is possible.
--
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-7323] Result of cast Number to Boolean is not correct [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4679:
URL: https://github.com/apache/calcite/pull/4679#discussion_r2613665578
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -825,6 +825,18 @@ void testCastToExactNumeric(CastType castType, SqlOperatorFixture f) {
f.checkNull("CAST(CAST(NULL AS VARCHAR) AS VARBINARY)");
}
+ /**
+ * Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-7323">
+ * Result of cast Number to Boolean is not correct</a>. */
+ @Test public void testNumbericBooleanCast() {
+ SqlOperatorFixture f = fixture();
+ f.checkScalar("CAST(1 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(0 AS BOOLEAN)", "false", "BOOLEAN NOT NULL");
+ f.checkScalar("CAST(1.2 AS BOOLEAN)", "true", "BOOLEAN NOT NULL");
Review Comment:
I added some test cases related to ```NULL``` and ```Double```, and the results should be as expected.
I added the variable type to ```RelToSqlConverterTest``` (which will convert it to var <>0), but I don't seem to fully understand the test way for ```-0e0```.
--
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