Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CALCITE-6074] The size of REAL, DOUBLE, and FLOAT is not consistent #3492

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

mihaibudiu
Copy link
Contributor

This is based on a code audit.
The assumption is that FLOAT=DOUBLE=64 bit and REAL=32 bit
I also deleted some deprecated functions instead of fixing them.

@mihaibudiu
Copy link
Contributor Author

Does anyone want to review this PR?
It relies on the core assumption that in Calcite FLOAT=DOUBLE.

This PR clearly fixes multiple bugs caused by misunderstandings about the floating point types.
In our compiler backend we have disabled the FLOAT type altogether (using a shuttle), because I think it's too dangerous for users to use correctly. My original proposal was to remove the FLOAT type from the Calcite IR and generate DOUBLE in the parser. Then such mistakes would become impossible in the future. This PR is less radical.

@dssysolyatin
Copy link
Contributor

@mihaibudiu PR looks good for me. I faced with the same problem when tried to convert sql from postgresql dialect to spark dialect. I was really surprise that spark float is 4 byte and postgres float is double precision by default.

@@ -704,82 +686,6 @@ public static long getMaxValue(RelDataType type) {
}
}

/** Returns whether a type has a representation as a Java primitive (ignoring
* nullability). */
@Deprecated // to be removed before 2.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently I can't delete deprecated functions, so I will put these back.

@mihaibudiu
Copy link
Contributor Author

I am planning to merge this PR soon. If you have comments now is the time.
It touches a lot of different parts.

Copy link

sonarcloud bot commented Dec 14, 2023

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

6 New issues
0 Security Hotspots
57.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@mihaibudiu mihaibudiu merged commit 08f6856 into apache:main Dec 14, 2023
17 checks passed
@mihaibudiu mihaibudiu deleted the issue6074 branch December 14, 2023 19:19
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