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/05/01 23:16:31 UTC
[PR] [CALCITE-6395] Significant precision loss when representing REAL literals [calcite]
mihaibudiu opened a new pull request, #3782:
URL: https://github.com/apache/calcite/pull/3782
(no comment)
--
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-6395] Significant precision loss when representing REAL literals [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3782:
URL: https://github.com/apache/calcite/pull/3782#discussion_r1594350263
##########
core/src/main/java/org/apache/calcite/rex/RexBuilder.java:
##########
@@ -1887,11 +1887,6 @@ public RexNode makeLambdaCall(RexNode expr, List<RexLambdaRef> parameters) {
type.getSqlTypeName());
return new BigDecimal(((Number) o).longValue());
case REAL:
- if (o instanceof BigDecimal) {
- return o;
- }
- return new BigDecimal(((Number) o).doubleValue(), MathContext.DECIMAL32)
- .stripTrailingZeros();
Review Comment:
Frankly I don't understand why in that place the float has to be converted to a decimal and then to a string. One can do the conversion directly. The question is whether I fix this in this PR or file another issue. If it's small enough I will bundle it here.
--
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-6395] Significant precision loss when representing REAL literals [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3782:
URL: https://github.com/apache/calcite/pull/3782#issuecomment-2101430614
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3782) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3782&resolved=false&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3782&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3782&resolved=false&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3782&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3782&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3782)
--
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-6395] Significant precision loss when representing REAL literals [calcite]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #3782:
URL: https://github.com/apache/calcite/pull/3782#issuecomment-2101273321
Thanks for fixing that, also checked that there is no more usages of `MathContext.DECIMAL32` in code
--
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-6395] Significant precision loss when representing REAL literals [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #3782:
URL: https://github.com/apache/calcite/pull/3782
--
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-6395] Significant precision loss when representing REAL literals [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3782:
URL: https://github.com/apache/calcite/pull/3782#discussion_r1594414196
##########
core/src/main/java/org/apache/calcite/rex/RexBuilder.java:
##########
@@ -1887,11 +1887,6 @@ public RexNode makeLambdaCall(RexNode expr, List<RexLambdaRef> parameters) {
type.getSqlTypeName());
return new BigDecimal(((Number) o).longValue());
case REAL:
- if (o instanceof BigDecimal) {
- return o;
- }
- return new BigDecimal(((Number) o).doubleValue(), MathContext.DECIMAL32)
- .stripTrailingZeros();
Review Comment:
The truth is that float only has about 7 decimal digits of precision, so one could argue that the original answer provided by Calcite was legal. However, it makes it much easier to test code if the result provided by Calcite for `CAST(CAST string AS REAL) AS BIGINT)` is the same as the Java result of `(long)Float.parseFloat(string)`, at least when running on the same CPU.
--
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-6395] Significant precision loss when representing REAL literals [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3782:
URL: https://github.com/apache/calcite/pull/3782#discussion_r1594420157
##########
core/src/main/java/org/apache/calcite/rex/RexBuilder.java:
##########
@@ -1887,11 +1887,6 @@ public RexNode makeLambdaCall(RexNode expr, List<RexLambdaRef> parameters) {
type.getSqlTypeName());
return new BigDecimal(((Number) o).longValue());
case REAL:
- if (o instanceof BigDecimal) {
- return o;
- }
- return new BigDecimal(((Number) o).doubleValue(), MathContext.DECIMAL32)
- .stripTrailingZeros();
Review Comment:
I have fixed this path as well in a subsequent commit.
Just because Calcite uses Decimal literals to represent FP values doesn't mean it has to convert to Decimal when converting to strings.
--
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-6395] Significant precision loss when representing REAL literals [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3782:
URL: https://github.com/apache/calcite/pull/3782#issuecomment-2089300020
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3782) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3782&resolved=false&inNewCodePeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3782&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3782&resolved=false&inNewCodePeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3782&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3782&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3782)
--
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-6395] Significant precision loss when representing REAL literals [calcite]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #3782:
URL: https://github.com/apache/calcite/pull/3782#discussion_r1593862214
##########
core/src/main/java/org/apache/calcite/rex/RexBuilder.java:
##########
@@ -1887,11 +1887,6 @@ public RexNode makeLambdaCall(RexNode expr, List<RexLambdaRef> parameters) {
type.getSqlTypeName());
return new BigDecimal(((Number) o).longValue());
case REAL:
- if (o instanceof BigDecimal) {
- return o;
- }
- return new BigDecimal(((Number) o).doubleValue(), MathContext.DECIMAL32)
- .stripTrailingZeros();
Review Comment:
I wonder whether will we have similar issue with
```sql
CAST(FLOAT AS VARCHAR)
```
https://github.com/apache/calcite/blob/19debd3e90e391bc6ba8250ec81735490b4b0cee/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java#L3511-L3520
--
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-6395] Significant precision loss when representing REAL literals [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3782:
URL: https://github.com/apache/calcite/pull/3782#issuecomment-2099125976
This PR actually *removes* some code to fix a bug. Should be easy to review!
--
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