sql/parser: avoid float -> unsigned conversions #15847
Conversation
|
This change is |
tamird
changed the title
While investigating this, I discovered that our current `round` implementation is quite dubious, and does not rely on any referenced source material or any material that I could find. I also found that Postgres does not implement 2-ary `round` where the first argument is a float. The above is addressed by: - replacing `round(float)` with a transcription of Postgres' `rint`. - replacing `round(float, int)` with an implementation that round-trips through apd. This is likely much slower, but likely correct. Updates #14405.
tamird
changed the title
|
LGTM, but Matt should chime in as well. What do you mean pg does not support this function? Do you mean it's not supported on floats? I'm pretty sure most SQL databases do support a round function with some semantics, and when I was looking at this last at least Sybase, Oracle and MSSQL were pretty explicit about what they are doing. |
Exactly, |
|
It would not hurt me to simply remove the round overload for floats altogether. People who really care about rounding on floats usually know what they want, and we support floor and ceil already. |
|
Understood, yet that would be a breaking change. |
|
Ack. Your current change moves us forward anyway. |
|
Review status: 0 of 4 files reviewed at latest revision, all discussions resolved, all commit checks successful. Comments from Reviewable |
|
We should cherry pick this into 1.0.1; the previous behaviour was definitely incorrect. |
While investigating this, I discovered that our current
roundimplementation is quite dubious, and does not rely on any referenced
source material or any material that I could find. I also found that
Postgres does not implement 2-ary
roundwhere the first argument isa float.
The above is addressed by:
round(float)with a transcription of Postgres'rint.round(float, int)with an implementation that round-tripsthrough apd. This is likely much slower, but likely correct.
FOR DOCS: in addition to the two points above, mention in docs that users that wish to have simple, straightforward results with floats are encouraged to use ceil() or floor() if applicable in their code, or possibly floor(x+0.5) if they want "round to nearest".
Updates #14405.
cc @vielmetti