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 2024/01/31 21:52:53 UTC
[PR] [CALCITE-2067] RexBuilder can't handle NaN,Infinity double constants [calcite]
mihaibudiu opened a new pull request, #3663:
URL: https://github.com/apache/calcite/pull/3663
This is another attempt to fix [CALCITE-2067].
PR #572 attempted to do this, but wasn't merged about 8 years ago.
This PR includes very similar changes.
The current implementation of RexLiteral uses a BigDecimal value to store DOUBLE values. This is a problem for 2 reasons:
- it loses precision, since some FP values cannot be stored precisely in decimal notation
- it cannot represent special FP values, such as -0.0, Infinity, or NaN
With this PR these issues are solved. As a side-effect, this enables the simplification engine to simplify some expressions that it couldn't before, such as expressions that produce special FP values.
Objections to the original PR which can be seen here: https://issues.apache.org/jira/browse/CALCITE-2067 include the fact that this "changes the SQL language accepted by Calcite." I want to preempt this objection: this change does not affect the parser or SqlNode, only RexNode. So it is only useful for *evaluation* and *optimization*. Moreover, if we will add support to the parser for special values as suggested in https://issues.apache.org/jira/browse/CALCITE-6058, we will certainly need a way to represent them in the RelNode IR. I claim that the changes in this PR are necessary for that goal.
The most unpleasant effect of this PR is to slightly change the semantics of some programs that used FP values before. These programs were in fact buggy, but third parties may have taken dependencies on these bugs. This kind of behavior was discovered by comparing results produced by Postgres (or Java) with Calcite on FP functions; the results were slightly off. The BigDecimal-induced rounding was the problem.
I also think that the constructor for RexLiteral that creates a literal for a DOUBLE type from a BigDecimal should be deprecated. I didn't do it in this 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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-2264068520
This PR is also fixing a 9-year feature request in Calcite.
From some remarks made by @julianhyde (which unfortunately I cannot find online) I deduce that he doesn't object to this change anymore.
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1473703079
##########
core/src/main/java/org/apache/calcite/util/Util.java:
##########
@@ -544,6 +544,21 @@ public static void println(
pw.println();
}
+ /**
+ * Formats a double value to a String ensuring that the output
+ * is in scientific notation if the value is not "special".
+ * (Special values include infinities and NaN.)
+ */
+ public static String toScientificNotation(Double d) {
Review Comment:
I realize that my previous message is wrong, indeed, there is no way to specify a binary floating point value in other SQL dialects. I don't think we can solve this problem, but we didn't introduce this problem in this PR. The problem exists already if one allows simplifications that can produce FP results.
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1475414363
##########
core/src/main/java/org/apache/calcite/rex/RexBuilder.java:
##########
@@ -1110,6 +1110,14 @@ public RexLiteral makeApproxLiteral(BigDecimal bd) {
return makeApproxLiteral(bd, typeFactory.createSqlType(SqlTypeName.DOUBLE));
}
+ /**
Review Comment:
I have deleted the 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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-1962225602
@julianhyde I think you have requested changes, I hope I have addressed your comments.
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1771839265
##########
core/src/test/java/org/apache/calcite/rex/RexExecutorTest.java:
##########
@@ -389,11 +389,14 @@ interface Action {
final RexCall first =
(RexCall) rexBuilder.makeCall(SqlStdOperatorTable.LN,
rexBuilder.makeLiteral(3, integer, true));
+ // Division by zero causes an exception during evaluation
final RexCall second =
- (RexCall) rexBuilder.makeCall(SqlStdOperatorTable.LN,
- rexBuilder.makeLiteral(-2, integer, true));
+ (RexCall) rexBuilder.makeCall(SqlStdOperatorTable.DIVIDE_INTEGER,
+ rexBuilder.makeLiteral(-2, integer, true),
+ rexBuilder.makeLiteral(0, integer, true));
executor.reduce(rexBuilder, ImmutableList.of(first, second),
reducedValues);
+ System.out.println(reducedValues);
Review Comment:
Let's remove this `println`
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-1921959086
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663) **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=3663&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3663&resolved=false&inNewCodePeriod=true)
[58.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_coverage&view=list)
[25.7% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663)
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-1923545735
bq. This is a breaking change in some respect [...]
Just a minor reminder: before the final merge, @mihaibudiu , I think it'd be necessary to include the corresponding comment about this on `history.md` in the "breaking changes" section for the next release 1.37 (so that we don't forget to mention it when preparing the next release).
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1529043905
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -4065,7 +4065,7 @@ private SqlDialect nonOrdinalDialect() {
+ " select \"product_id\", 0 as \"net_weight\"\n"
+ " from \"sales_fact_1997\") t0";
final String expected = "SELECT SUM(CASE WHEN \"product_id\" = 0"
- + " THEN \"net_weight\" ELSE 0 END) AS \"NET_WEIGHT\"\n"
+ + " THEN \"net_weight\" ELSE 0E0 END) AS \"NET_WEIGHT\"\n"
Review Comment:
I think we'd be hard-pressed to have users use this literal FYI
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -388,7 +388,7 @@ private static String toSql(RelNode root, SqlDialect dialect,
+ "where \"net_weight\" <> 10 or \"net_weight\" is null";
final String expected = "SELECT \"product_id\", \"shelf_width\"\n"
+ "FROM \"foodmart\".\"product\"\n"
- + "WHERE \"net_weight\" <> 10 OR \"net_weight\" IS NULL";
+ + "WHERE \"net_weight\" <> CAST(10 AS DOUBLE) OR \"net_weight\" IS NULL";
Review Comment:
Would users need to make changes to queries such as these? Is 10 no longer implicitly converted to double when comparing to double columns?
--
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-2067] RexBuilder can't handle NaN,Infinity double constants [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-1920086516
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[10 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3663&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3663&resolved=false&inNewCodePeriod=true)
[71.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_coverage&view=list)
[24.6% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663)
--
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-2067] RexBuilder can't handle NaN,Infinity double constants [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1473677633
##########
core/src/main/java/org/apache/calcite/rex/RexLiteral.java:
##########
@@ -325,19 +324,18 @@ public static boolean valueMatchesType(
}
// fall through
case DECIMAL:
+ case BIGINT:
+ return value instanceof BigDecimal;
case DOUBLE:
case FLOAT:
case REAL:
- case BIGINT:
- return value instanceof BigDecimal;
+ return value instanceof BigDecimal || value instanceof Double;
Review Comment:
Indeed, the decimal option should be removed, but I kept it for backward compatibility with third-party users of Calcite.
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #3663:
URL: https://github.com/apache/calcite/pull/3663
--
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-2067] RexBuilder can't handle NaN,Infinity double constants [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1473677915
##########
core/src/main/java/org/apache/calcite/rex/RexBuilder.java:
##########
@@ -1110,6 +1110,14 @@ public RexLiteral makeApproxLiteral(BigDecimal bd) {
return makeApproxLiteral(bd, typeFactory.createSqlType(SqlTypeName.DOUBLE));
}
+ /**
Review Comment:
Yes, this should be deprecated. Can I do 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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1771726795
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1498,7 +1498,7 @@ public static SqlNode toSql(RexLiteral literal) {
case EXACT_NUMERIC: {
if (SqlTypeName.APPROX_TYPES.contains(typeName)) {
return SqlLiteral.createApproxNumeric(
- castNonNull(literal.getValueAs(BigDecimal.class)).toPlainString(), POS);
+ castNonNull(literal.getValueAs(Double.class)).toString(), POS);
Review Comment:
Maybe we should add Javadoc to prevent someone from changing it back to BigDecimal later.
##########
arrow/src/main/java/org/apache/calcite/adapter/arrow/ArrowTranslator.java:
##########
@@ -228,6 +228,8 @@ private static String getLiteralType(Object literal) {
}
} else if (String.class.equals(literal.getClass())) {
return "string";
+ } else if (literal instanceof Double) {
+ return "float";
Review Comment:
why `float`?
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-2368734415
Overall LGTM
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1771842919
##########
arrow/src/test/java/org/apache/calcite/adapter/arrow/ArrowAdapterTest.java:
##########
@@ -373,7 +373,7 @@ static void initializeArrowState(@TempDir Path sharedTempDir)
String sql = "select * from arrowdata\n"
+ " where \"floatField\"=15.0";
String plan = "PLAN=ArrowToEnumerableConverter\n"
- + " ArrowFilter(condition=[=(CAST($2):DOUBLE, 15.0)])\n"
+ + " ArrowFilter(condition=[=(CAST($2):DOUBLE, 15.0E0)])\n"
Review Comment:
I see a lot of `E0` added. Is it a goal of this change to make decimal literals print differently from double literals?
Maybe I missed it, but somewhere in the javadoc (e.g. for RexLiteral) should state that goal.
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-2364886184
By reviewing this PR you can help close a 9-year issue for Calcite!
The changes look much bigger than they are because tests are affected, but the logic is actually very simple.
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1771897546
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1498,7 +1498,7 @@ public static SqlNode toSql(RexLiteral literal) {
case EXACT_NUMERIC: {
if (SqlTypeName.APPROX_TYPES.contains(typeName)) {
return SqlLiteral.createApproxNumeric(
- castNonNull(literal.getValueAs(BigDecimal.class)).toPlainString(), POS);
+ castNonNull(literal.getValueAs(Double.class)).toString(), POS);
Review Comment:
It might crash today, but it's possible that someone will enhance `RexLiteral.getValueAs` to accept `BigDecimal.class` as an argument. That's not an unreasonable change.
I had different concerns about this code, namely that assumes that how Java formats Double values is the same as how SQL dialects represent FLOAT literals.
I am satisfied that code is OK only because there are already quite extensive unit tests for SQL generation.
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-2369975760
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663) **Quality Gate passed**
Issues
 [13 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3663&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3663&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3663&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [71.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663)
--
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-2067] RexBuilder can't handle NaN,Infinity double constants [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1473680350
##########
core/src/main/java/org/apache/calcite/util/Util.java:
##########
@@ -544,6 +544,21 @@ public static void println(
pw.println();
}
+ /**
+ * Formats a double value to a String ensuring that the output
+ * is in scientific notation if the value is not "special".
+ * (Special values include infinities and NaN.)
+ */
+ public static String toScientificNotation(Double d) {
Review Comment:
Unfortunately there is no bijection. This conversion is lossy in both directions, from String to Double and from Double to String. This method is not 100% adequate for generating SQL, but it is adequate for e.g., displaying plans.
However, the lossy conversion problem already exists in Calcite and is not specific to DOUBLE values: see https://issues.apache.org/jira/browse/CALCITE-5987. I have discussed in a message on the dev list about how this separate issue can be tackled.
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1771852575
##########
arrow/src/main/java/org/apache/calcite/adapter/arrow/ArrowTranslator.java:
##########
@@ -228,6 +228,8 @@ private static String getLiteralType(Object literal) {
}
} else if (String.class.equals(literal.getClass())) {
return "string";
+ } else if (literal instanceof Double) {
+ return "float";
Review Comment:
this is the same string that is used 3 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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1771836704
##########
core/src/test/java/org/apache/calcite/util/UtilTest.java:
##########
@@ -209,6 +215,73 @@ class UtilTest {
Util.toScientificNotation(bd));
}
+ @Test void testDoubleScientificNotation() {
+ double d = parseDouble("0.001234");
+ TestUtil.assertEqualsVerbose(
+ "0.001234E0",
+ Util.toScientificNotation(d));
+ d = parseDouble("0.001");
Review Comment:
I find that redundancy makes it difficult to see patterns in tests. Consider introducing a helper method, so that
```
d = parseDouble("0.001234");
TestUtil.assertEqualsVerbose(
"0.001234E0",
Util.toScientificNotation(d));
```
becomes
```
assertThatScientific("0.001234", is("0.001234E0"));
```
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-2159084319
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663) **Quality Gate passed**
Issues
 [13 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3663&resolved=false&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3663&resolutions=WONTFIX)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3663&resolved=false&sinceLeakPeriod=true)
 [70.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663)
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1529048909
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -388,7 +388,7 @@ private static String toSql(RelNode root, SqlDialect dialect,
+ "where \"net_weight\" <> 10 or \"net_weight\" is null";
final String expected = "SELECT \"product_id\", \"shelf_width\"\n"
+ "FROM \"foodmart\".\"product\"\n"
- + "WHERE \"net_weight\" <> 10 OR \"net_weight\" IS NULL";
+ + "WHERE \"net_weight\" <> CAST(10 AS DOUBLE) OR \"net_weight\" IS NULL";
Review Comment:
no, this is just the canonical representation of the program created by Calcite which proves that the 10 is implicitly cast to a double
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1529049170
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -4065,7 +4065,7 @@ private SqlDialect nonOrdinalDialect() {
+ " select \"product_id\", 0 as \"net_weight\"\n"
+ " from \"sales_fact_1997\") t0";
final String expected = "SELECT SUM(CASE WHEN \"product_id\" = 0"
- + " THEN \"net_weight\" ELSE 0 END) AS \"NET_WEIGHT\"\n"
+ + " THEN \"net_weight\" ELSE 0E0 END) AS \"NET_WEIGHT\"\n"
Review Comment:
same as above, they don't have to.
--
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-2067] RexBuilder can't handle NaN,Infinity double constants [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1473677915
##########
core/src/main/java/org/apache/calcite/rex/RexBuilder.java:
##########
@@ -1110,6 +1110,14 @@ public RexLiteral makeApproxLiteral(BigDecimal bd) {
return makeApproxLiteral(bd, typeFactory.createSqlType(SqlTypeName.DOUBLE));
}
+ /**
Review Comment:
Yes, this should be deprecated. Can I do 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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1475414494
##########
core/src/main/java/org/apache/calcite/rex/RexLiteral.java:
##########
@@ -325,19 +324,18 @@ public static boolean valueMatchesType(
}
// fall through
case DECIMAL:
+ case BIGINT:
+ return value instanceof BigDecimal;
case DOUBLE:
case FLOAT:
case REAL:
- case BIGINT:
- return value instanceof BigDecimal;
+ return value instanceof BigDecimal || value instanceof Double;
Review Comment:
I have removed the choice here 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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-1924584002
@rubenada I have added a commented-out breaking change for the next release. Thank you for the reminder.
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-1922604110
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3663&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3663&resolved=false&inNewCodePeriod=true)
[70.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663)
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1772060274
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1498,7 +1498,7 @@ public static SqlNode toSql(RexLiteral literal) {
case EXACT_NUMERIC: {
if (SqlTypeName.APPROX_TYPES.contains(typeName)) {
return SqlLiteral.createApproxNumeric(
- castNonNull(literal.getValueAs(BigDecimal.class)).toPlainString(), POS);
+ castNonNull(literal.getValueAs(Double.class)).toString(), POS);
Review Comment:
BigDecimal.class should still be a legal argument for this function.
There are a few places in the code where this happens.
This code assumes only that the createApproxNumeric will correctly handle Java string representations of double 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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-2369371343
I have pushed a new commit which hopefully addresses the review comments.
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-2264123265
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663) **Quality Gate passed**
Issues
 [12 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3663&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3663&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3663&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [70.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663)
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1771875406
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1498,7 +1498,7 @@ public static SqlNode toSql(RexLiteral literal) {
case EXACT_NUMERIC: {
if (SqlTypeName.APPROX_TYPES.contains(typeName)) {
return SqlLiteral.createApproxNumeric(
- castNonNull(literal.getValueAs(BigDecimal.class)).toPlainString(), POS);
+ castNonNull(literal.getValueAs(Double.class)).toString(), POS);
Review Comment:
If they change this to BigDecimal the program will crash with a type cast error.
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-2370007426
This PR has been approved in JIRA, so I will merge 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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-2368679361
@mihaibudiu I will review this PR, before version 1.38.0, which is expected to be completed tomorrow
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1771844641
##########
arrow/src/test/java/org/apache/calcite/adapter/arrow/ArrowAdapterTest.java:
##########
@@ -373,7 +373,7 @@ static void initializeArrowState(@TempDir Path sharedTempDir)
String sql = "select * from arrowdata\n"
+ " where \"floatField\"=15.0";
String plan = "PLAN=ArrowToEnumerableConverter\n"
- + " ArrowFilter(condition=[=(CAST($2):DOUBLE, 15.0)])\n"
+ + " ArrowFilter(condition=[=(CAST($2):DOUBLE, 15.0E0)])\n"
Review Comment:
I haven't changed how floating point values are serialized as text. The difference comes because now more literals contain a double/float.
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1771849943
##########
arrow/src/test/java/org/apache/calcite/adapter/arrow/ArrowAdapterTest.java:
##########
@@ -373,7 +373,7 @@ static void initializeArrowState(@TempDir Path sharedTempDir)
String sql = "select * from arrowdata\n"
+ " where \"floatField\"=15.0";
String plan = "PLAN=ArrowToEnumerableConverter\n"
- + " ArrowFilter(condition=[=(CAST($2):DOUBLE, 15.0)])\n"
+ + " ArrowFilter(condition=[=(CAST($2):DOUBLE, 15.0E0)])\n"
Review Comment:
Makes sense.
+1 when you fix the issues in my other 2 comments
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-2065453164
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663) **Quality Gate passed**
Issues
 [12 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3663&resolved=false&inNewCodePeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3663&resolved=false&inNewCodePeriod=true)
 [70.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663)
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-1922637894
@julianhyde I hope to have addressed your comments.
This is a breaking change in some respect; one can see how the Druid Adapter code had to be modified to accommodate this change. But I think it fixes a genuine bug. It doesn't solve the problem of FP literals completely, but it enables more programs to produce correct results than before.
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1529038987
##########
core/src/main/java/org/apache/calcite/rex/RexLiteral.java:
##########
@@ -1060,22 +1065,46 @@ public boolean isNull() {
case BIGINT:
case INTEGER:
case SMALLINT:
- case TINYINT:
- case DOUBLE:
- case REAL:
- case FLOAT:
+ case TINYINT: {
+ BigDecimal bd = (BigDecimal) value;
if (clazz == Long.class) {
- return clazz.cast(((BigDecimal) value).longValue());
+ return clazz.cast(bd.longValue());
} else if (clazz == Integer.class) {
- return clazz.cast(((BigDecimal) value).intValue());
+ return clazz.cast(bd.intValue());
} else if (clazz == Short.class) {
- return clazz.cast(((BigDecimal) value).shortValue());
+ return clazz.cast(bd.shortValue());
} else if (clazz == Byte.class) {
- return clazz.cast(((BigDecimal) value).byteValue());
+ return clazz.cast(bd.byteValue());
} else if (clazz == Double.class) {
- return clazz.cast(((BigDecimal) value).doubleValue());
+ return clazz.cast(bd.doubleValue());
} else if (clazz == Float.class) {
- return clazz.cast(((BigDecimal) value).floatValue());
+ return clazz.cast(bd.floatValue());
+ }
+ break;
+ }
+ case DOUBLE:
+ case REAL:
+ case FLOAT:
+ if (value instanceof Double) {
+ Double d = (Double) value;
+ if (clazz == Long.class) {
+ return clazz.cast(d.longValue());
+ } else if (clazz == Integer.class) {
+ return clazz.cast(d.intValue());
+ } else if (clazz == Short.class) {
+ return clazz.cast(d.shortValue());
+ } else if (clazz == Byte.class) {
+ return clazz.cast(d.byteValue());
+ } else if (clazz == Double.class) {
+ return clazz.cast(d);
Review Comment:
Isn't this just d?
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-1924646186
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3663&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3663&resolved=false&inNewCodePeriod=true)
[70.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663)
--
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-2067] RexBuilder can't handle NaN,Infinity double constants [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-1920176323
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[10 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3663&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3663&resolved=false&inNewCodePeriod=true)
[71.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_coverage&view=list)
[24.6% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663)
--
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-2067] RexBuilder can't handle NaN,Infinity double constants [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1473635196
##########
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##########
@@ -3027,6 +3027,31 @@ private void checkPushJoinThroughUnionOnRightDoesNotMatchSemiOrAntiJoin(JoinRelT
.check();
}
+ /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-2067">
Review Comment:
Conventional format includes `[`.
##########
core/src/main/java/org/apache/calcite/util/Util.java:
##########
@@ -544,6 +544,21 @@ public static void println(
pw.println();
}
+ /**
+ * Formats a double value to a String ensuring that the output
+ * is in scientific notation if the value is not "special".
+ * (Special values include infinities and NaN.)
+ */
+ public static String toScientificNotation(Double d) {
Review Comment:
Need unit tests for this method.
##########
core/src/main/java/org/apache/calcite/rex/RexLiteral.java:
##########
@@ -325,19 +324,18 @@ public static boolean valueMatchesType(
}
// fall through
case DECIMAL:
+ case BIGINT:
+ return value instanceof BigDecimal;
case DOUBLE:
case FLOAT:
case REAL:
- case BIGINT:
- return value instanceof BigDecimal;
+ return value instanceof BigDecimal || value instanceof Double;
Review Comment:
Is there anything to be gained in giving people a choice?
##########
core/src/main/java/org/apache/calcite/rex/RexBuilder.java:
##########
@@ -1110,6 +1110,14 @@ public RexLiteral makeApproxLiteral(BigDecimal bd) {
return makeApproxLiteral(bd, typeFactory.createSqlType(SqlTypeName.DOUBLE));
}
+ /**
Review Comment:
I think this method is too easy to invoke by accident.
##########
core/src/main/java/org/apache/calcite/util/Util.java:
##########
@@ -544,6 +544,21 @@ public static void println(
pw.println();
}
+ /**
+ * Formats a double value to a String ensuring that the output
+ * is in scientific notation if the value is not "special".
+ * (Special values include infinities and NaN.)
+ */
+ public static String toScientificNotation(Double d) {
Review Comment:
Should this method guarantee that there is a unique string for every possible `double` value? (Except for the many representations of `NaN` etc.) And that it has an inverse method that will convert the string to the same `double` value.
I believe that we are relying on this bijection, because there is no direct way to specify a binary floating point value in SQL.
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-1920440503
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[11 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3663&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3663&resolved=false&inNewCodePeriod=true)
[58.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_coverage&view=list)
[25.7% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663)
--
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-2067] RexBuilder can't handle NaN,Infinity double constants [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-1920145454
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[10 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3663&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3663&resolved=false&inNewCodePeriod=true)
[71.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_coverage&view=list)
[24.6% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663)
--
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-2067] RexBuilder can't handle NaN,Infinity double constants [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-1920040279
@julianhyde you had most objections to the previous PR addressing this issue. I hope that you will find my arguments adequate.
--
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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3663:
URL: https://github.com/apache/calcite/pull/3663#discussion_r1529449543
##########
core/src/main/java/org/apache/calcite/rex/RexLiteral.java:
##########
@@ -1060,22 +1065,46 @@ public boolean isNull() {
case BIGINT:
case INTEGER:
case SMALLINT:
- case TINYINT:
- case DOUBLE:
- case REAL:
- case FLOAT:
+ case TINYINT: {
+ BigDecimal bd = (BigDecimal) value;
if (clazz == Long.class) {
- return clazz.cast(((BigDecimal) value).longValue());
+ return clazz.cast(bd.longValue());
} else if (clazz == Integer.class) {
- return clazz.cast(((BigDecimal) value).intValue());
+ return clazz.cast(bd.intValue());
} else if (clazz == Short.class) {
- return clazz.cast(((BigDecimal) value).shortValue());
+ return clazz.cast(bd.shortValue());
} else if (clazz == Byte.class) {
- return clazz.cast(((BigDecimal) value).byteValue());
+ return clazz.cast(bd.byteValue());
} else if (clazz == Double.class) {
- return clazz.cast(((BigDecimal) value).doubleValue());
+ return clazz.cast(bd.doubleValue());
} else if (clazz == Float.class) {
- return clazz.cast(((BigDecimal) value).floatValue());
+ return clazz.cast(bd.floatValue());
+ }
+ break;
+ }
+ case DOUBLE:
+ case REAL:
+ case FLOAT:
+ if (value instanceof Double) {
+ Double d = (Double) value;
+ if (clazz == Long.class) {
+ return clazz.cast(d.longValue());
+ } else if (clazz == Integer.class) {
+ return clazz.cast(d.intValue());
+ } else if (clazz == Short.class) {
+ return clazz.cast(d.shortValue());
+ } else if (clazz == Byte.class) {
+ return clazz.cast(d.byteValue());
+ } else if (clazz == Double.class) {
+ return clazz.cast(d);
Review Comment:
Yes, it is, but the return type must be T, so the cast is necessary anyway. The Java compiler isn't smart enough to understand 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-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3663:
URL: https://github.com/apache/calcite/pull/3663#issuecomment-2365011241
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663) **Quality Gate passed**
Issues
 [13 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3663&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3663&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3663&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [70.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3663&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3663)
--
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