sql: fix float to integer casting #117798

da-ket · 2024-01-16T08:39:30Z

Previously, casting a float value to an integer simply truncates the float. This commit fixes that by rounding the float value to the nearest integer, rounding ties to even, as in postgres.

Epic: None
Fixes: #112515

Release note (bug fix): A bug has been fixed where casts of floats to integers
truncated the value. These casts now round a float to the nearest integer,
rounding ties to even integers.

blathers-crl · 2024-01-16T08:39:34Z

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2024-01-16T08:39:37Z

This change is

da-ket

Hello! The updated test code all matches with postgres.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)

pkg/ccl/logictestccl/testdata/logic_test/udf_plpgsql line 2419 at r1 (raw file):

SELECT f();
----
106

postgres=# CREATE OR REPLACE FUNCTION f() RETURNS INT AS $$
postgres$#   BEGIN
postgres$#     RETURN 105.67::FLOAT;
postgres$#   END  
postgres$# $$ LANGUAGE PLpgSQL;
CREATE FUNCTION
postgres=# SELECT f();
  f  
-----
 106
(1 row)

pkg/sql/logictest/testdata/logic_test/aggregate line 992 at r1 (raw file):

SELECT round(variance(n), 2), round(variance(n), 2), round(variance(p)) FROM mnop
----
0.08 0.08 9

postgres=# CREATE TABLE mnop (
postgres(#   m INT PRIMARY KEY, 
postgres(#   n FLOAT,
postgres(#   o DECIMAL,
postgres(#   p BIGINT
postgres(# )
postgres-# ;
CREATE TABLE
postgres=# INSERT INTO mnop (m, n) SELECT i, (1e9 + i/100.0)::float FROM 
postgres-#   generate_series(1, 100) AS i(i) ;
INSERT 0 100
postgres=# UPDATE mnop SET o = n::decimal, p = (n * 10)::bigint;
UPDATE 100
postgres=# select round(variance(p)) from mnop;
 round 
-------
     9
(1 row)

pkg/sql/logictest/testdata/logic_test/aggregate line 997 at r1 (raw file):

SELECT round(var_pop(n), 2), round(var_pop(n), 2), round(var_pop(p)) FROM mnop
----
0.08 0.08 9

postgres=# select round(var_pop(p)) FROM mnop;
 round 
-------
     9
(1 row)

pkg/sql/logictest/testdata/logic_test/target_names line 76 at r1 (raw file):

----
a   column1 cos
123 1       1

postgres=# SELECT (SELECT 123 AS a), 
postgres-#        (VALUES (cos(1)::INT)),
postgres-#        (SELECT cos(0)::INT);
  a  | column1 | cos 
-----+---------+-----
 123 |       1 |   1
(1 row)

mgartner

Thank you for the contribution! This looks great. I left one comment about the commit message.

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @da-ket and @DrewKimball)

-- commits line 9 at r1:
Can you add a release note at the bottom of the commit, like:

Release note (bug fix): A bug has been fix where casts of floats to integers truncated
the value. These casts now round a float to the nearest integer, rounding ties to even
integers.

blathers-crl · 2024-01-16T16:36:15Z

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

da-ket

Thank you for the review! I followed your guidance on writing the release notes.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @mgartner)

-- commits line 9 at r1:

Previously, mgartner (Marcus Gartner) wrote…

Can you add a release note at the bottom of the commit, like:

Release note (bug fix): A bug has been fix where casts of floats to integers truncated
the value. These casts now round a float to the nearest integer, rounding ties to even
integers.

Done.

da-ket

Hi @mgartner , just a gentle reminder about the pending review of this pull request.
Your feedback would be greatly appreciated!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @mgartner)

RaduBerinde · 2024-02-02T04:23:26Z

pkg/sql/colexec/execgen/cmd/execgen/cast_gen_util.go

@@ -271,7 +271,7 @@ func floatToInt(intWidth, floatWidth int32) castFunc {
 			if math.IsNaN(float64(%[2]s)) || %[2]s <= float%[4]d(math.MinInt%[3]d) || %[2]s >= float%[4]d(math.MaxInt%[3]d) {
 				colexecerror.ExpectedError(tree.ErrIntOutOfRange)
 			}
-			%[1]s = int%[3]d(%[2]s)
+			%[1]s = int%[3]d(math.RoundToEven(%[2]s))


Why RoundToEven? Does postgres do that?

Ah, I was reading the postgres docs for the ROUND function. I couldn't find specific documentation on casting.

da-ket

Thanks for the comments!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @RaduBerinde)

pkg/sql/colexec/execgen/cmd/execgen/cast_gen_util.go line 274 at r2 (raw file):

Previously, RaduBerinde wrote…

Ah, I was reading the postgres docs for the ROUND function. I couldn't find specific documentation on casting.

Yes, I used RoundToEven since postgres does do that when casting floats to integers. It isn't clearly mentioned in the documentation, but it can be found in their code for built-in functions for float type. PostgreSQL follows the round to nearest, ties to even rule for both rounding of float types and their casting to integers. CRDB was follows the same tie-breaking rule for rounding, but it just truncates when casting. I thought the issue #112515 wants to align this difference to be consistent.

RaduBerinde · 2024-02-02T10:17:31Z

pkg/sql/colexec/execgen/cmd/execgen/cast_gen_util.go line 274 at r2 (raw file):

Previously, da-ket (daket) wrote…

Yes, I used RoundToEven since postgres does do that when casting floats to integers. It isn't clearly mentioned in the documentation, but it can be found in their code for built-in functions for float type. PostgreSQL follows the round to nearest, ties to even rule for both rounding of float types and their casting to integers. CRDB was follows the same tie-breaking rule for rounding, but it just truncates when casting. I thought the issue #112515 wants to align this difference to be consistent.

Makes sense, thank you!

Previously, casting a float value to an integer simply truncates the float. This commit fixes that by rounding the float value to the nearest integer, rounding ties to even, as in postgres. Epic: None Fixes: cockroachdb#112515 Release note (bug fix): A bug has been fixed where casts of floats to integers truncated the value. These casts now round a float to the nearest integer, rounding ties to even integers.

mgartner

Looks great. Thanks again from contributing! I added a regression test for the change in the vectorized engine cast_gen_util.go. I'll go ahead and merge this once all the tests pass.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @da-ket and @DrewKimball)

mgartner · 2024-02-02T18:38:42Z

bors r+

craig · 2024-02-02T19:45:19Z

Build succeeded:

Bazel Essential CI (Cockroach)

da-ket requested a review from a team as a code owner January 16, 2024 08:39

da-ket requested review from DrewKimball and removed request for a team January 16, 2024 08:39

blathers-crl bot added the O-community Originated from the community label Jan 16, 2024

da-ket commented Jan 16, 2024

View reviewed changes

mgartner requested changes Jan 16, 2024

View reviewed changes

da-ket force-pushed the float_to_integer branch from 37fe58f to 2b6e917 Compare January 16, 2024 16:36

da-ket commented Jan 16, 2024

View reviewed changes

da-ket commented Feb 1, 2024

View reviewed changes

RaduBerinde reviewed Feb 2, 2024

View reviewed changes

da-ket commented Feb 2, 2024

View reviewed changes

mgartner force-pushed the float_to_integer branch from 2b6e917 to 66be939 Compare February 2, 2024 17:02

mgartner approved these changes Feb 2, 2024

View reviewed changes

mgartner mentioned this pull request Feb 2, 2024

Document FLOAT -> INT casting behavior cockroachdb/docs#18272

Open

craig bot merged commit 9434801 into cockroachdb:master Feb 2, 2024
8 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: fix float to integer casting #117798

sql: fix float to integer casting #117798

da-ket commented Jan 16, 2024 •

edited by mgartner

blathers-crl bot commented Jan 16, 2024

cockroach-teamcity commented Jan 16, 2024

da-ket left a comment

mgartner left a comment

blathers-crl bot commented Jan 16, 2024

da-ket left a comment

da-ket left a comment

RaduBerinde Feb 2, 2024

RaduBerinde Feb 2, 2024

da-ket left a comment

RaduBerinde commented Feb 2, 2024

mgartner left a comment

mgartner commented Feb 2, 2024

craig bot commented Feb 2, 2024

sql: fix float to integer casting #117798

sql: fix float to integer casting #117798

Conversation

da-ket commented Jan 16, 2024 • edited by mgartner

blathers-crl bot commented Jan 16, 2024

cockroach-teamcity commented Jan 16, 2024

da-ket left a comment

Choose a reason for hiding this comment

mgartner left a comment

Choose a reason for hiding this comment

blathers-crl bot commented Jan 16, 2024

da-ket left a comment

Choose a reason for hiding this comment

da-ket left a comment

Choose a reason for hiding this comment

RaduBerinde Feb 2, 2024

Choose a reason for hiding this comment

RaduBerinde Feb 2, 2024

Choose a reason for hiding this comment

da-ket left a comment

Choose a reason for hiding this comment

RaduBerinde commented Feb 2, 2024

mgartner left a comment

Choose a reason for hiding this comment

mgartner commented Feb 2, 2024

craig bot commented Feb 2, 2024

da-ket commented Jan 16, 2024 •

edited by mgartner