apache
[CALCITE-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity
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.
@julianhyde you had most objections to the previous PR addressing this issue. I hope that you will find my arguments adequate.
@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 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).
@rubenada I have added a commented-out breaking change for the next release. Thank you for the reminder.
Quality Gate passed
The SonarCloud Quality Gate passed, but some issues were introduced.
3 New issues
0 Security Hotspots
70.6% Coverage on New Code
0.0% Duplication on New Code
See analysis details on SonarCloud
![sonarqubecloud[bot] avatar](https://avatars.githubusercontent.com/in/12526?v=4 "Published by a pub.dev verified publisher"){kind=small}
@julianhyde I think you have requested changes, I hope I have addressed your comments.
Quality Gate passed
Issues
12 New issues
0 Accepted issues
Measures
0 Security Hotspots
70.8% Coverage on New Code
0.0% Duplication on New Code
See analysis details on SonarCloud
Quality Gate passed
Issues
13 New issues
0 Accepted issues
Measures
0 Security Hotspots
70.4% Coverage on New Code
0.0% Duplication on New Code
See analysis details on SonarCloud
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.
Quality Gate passed
Issues
12 New issues
0 Accepted issues
Measures
0 Security Hotspots
70.4% Coverage on New Code
0.0% Duplication on New Code
See analysis details on SonarCloud