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] 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] 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_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] 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] 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 "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] 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 "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-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-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 "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] 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] 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] 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] 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] 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] 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] 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