Skip to content

[CALCITE-6395] Significant precision loss when representing REAL literals#3782

Merged
mihaibudiu merged 1 commit into
apache:mainfrom
mihaibudiu:issue6395
May 8, 2024
Merged

[CALCITE-6395] Significant precision loss when representing REAL literals#3782
mihaibudiu merged 1 commit into
apache:mainfrom
mihaibudiu:issue6395

Conversation

@mihaibudiu
Copy link
Copy Markdown
Contributor

No description provided.

@mihaibudiu
Copy link
Copy Markdown
Contributor Author

This PR actually removes some code to fix a bug. Should be easy to review!

return o;
}
return new BigDecimal(((Number) o).doubleValue(), MathContext.DECIMAL32)
.stripTrailingZeros();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether will we have similar issue with

CAST(FLOAT AS VARCHAR)

/** CAST(FLOAT AS VARCHAR). */
public static String toString(float x) {
if (x == 0) {
return "0E0";
}
BigDecimal bigDecimal =
new BigDecimal(x, MathContext.DECIMAL32).stripTrailingZeros();
final String s = bigDecimal.toString();
return PATTERN_0_STAR_E.matcher(s).replaceAll("E").replace("E+", "E");
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@snuyanzin
Copy link
Copy Markdown
Contributor

Thanks for fixing that, also checked that there is no more usages of MathContext.DECIMAL32 in code

…rals

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 8, 2024

@mihaibudiu mihaibudiu merged commit b6e1a9d into apache:main May 8, 2024
@mihaibudiu mihaibudiu deleted the issue6395 branch May 8, 2024 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants