sql,pgwire: fixes for float formatting #82022

rafiss · 2022-05-28T19:24:00Z

sql/pgwire: update float formatting to be compatible with PG12

Release note (sql change): The extra_float_digits session variable now
defaults to 1. The meaning of the variable has also changed. Now, any
value greater than zero causes floats to be formatted in their
shortest precise decimal representation. That is, the string representation
produced is closer to the actual binary value than to any other value.
(Previously, this was only the behavior when extra_float_digits was set
to 3.) This change was made in accordance with an equivalent change that
was part of the PostgreSQL 12.0 release.

The behavior of a non-positive extra_float_digits value is unchanged. That
will still reduce the number of float digits shown in the output string.
The formula for number of digits shown is max(1, (DIGITS + extra_float_digits)),
where DIGITS=6 for FLOAT4 values, and DIGITS=15 for FLOAT8 values.

sql/pgwire: fix formatting of float arrays

Release note (bug fix): Fixed the formatting of floats in arrays sent
over the client-server pgwire protocol so that they respect the
extra_float_digits parameter, and correctly format infinity values.

eval: use common function for float->text cast

There's no behavior change, but this reduces code duplication.

This also adds a test that demonstrates a gap in our type/cast system.

cockroach-teamcity · 2022-05-28T19:24:09Z

This change is

knz

Nice - but I'll let oliver sign off.

Reviewed 6 of 6 files at r1, 3 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)

otan · 2022-05-29T03:46:57Z

pkg/sql/pgwire/testdata/pgtest/float

+
+
+send
+Query {"String": "SET extra_float_digits = -1"}


can you add a test for a positive extra_float_digits > 1?

otan · 2022-05-29T03:47:21Z

pkg/sql/pgwire/testdata/pgtest/float

+ReadyForQuery
+----
+{"Type":"RowDescription","Fields":[{"Name":"float4","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":700,"DataTypeSize":4,"TypeModifier":-1,"Format":0},{"Name":"float8","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":701,"DataTypeSize":8,"TypeModifier":-1,"Format":0}]}
+{"Type":"DataRow","Values":[{"text":"0.33333334"},{"text":"0.3333333333333333"}]}


ooh we round up to 4? is this consistent when running this with PG?

yep, the expected result was obtained by running against PG

otan · 2022-05-29T03:48:00Z

pkg/sql/sem/tree/pgwire_encode.go

@@ -162,6 +170,27 @@ func init() {
 	}
 }

+// PgwireFormatFloat returns a []byte representing a float according to
+// pgwire encoding. The result is appended to the given buffer.
+func PgwireFormatFloat(


wonder if this should be PGWire, but that's an optional nit

i was matching the naming of PgwireFormatBool

rafiss · 2022-05-30T02:26:44Z

bors r=otan

craig · 2022-05-30T03:12:46Z

Build failed:

GitHub CI (Cockroach)

Release note (sql change): The extra_float_digits session variable now defaults to 1. The meaning of the variable has also changed. Now, any value greater than zero causes floats to be formatted in their shortest precise decimal representation. That is, the string representation produced is closer to the actual binary value than to any other value. (Previously, this was only the behavior when extra_float_digits was set to 3.) This change was made in accordance with an equivalent change that was part of the PostgreSQL 12.0 release. The behavior of a non-positive extra_float_digits value is unchanged. That will still reduce the number of float digits shown in the output string. The formula for number of digits shown is `max(1, (DIGITS + extra_float_digits))`, where DIGITS=6 for FLOAT4 values, and DIGITS=15 for FLOAT8 values.

Release note (bug fix): Fixed the formatting of floats in arrays and tuples sent over the client-server pgwire protocol so that they respect the extra_float_digits parameter, and correctly format infinity values.

There's no behavior change, but this reduces code duplication. This also adds a test that demonstrates a gap in our type/cast system. Release note: None

The encoding tests were updated so they use the new default value of 1 for extra_float_digits. This makes the test more aligned to Postgres behavior. Release note: None

rafiss · 2022-05-30T04:00:37Z

bors r=otan

82022: sql,pgwire: fixes for float formatting r=otan a=rafiss ### sql/pgwire: update float formatting to be compatible with PG12 Release note (sql change): The extra_float_digits session variable now defaults to 1. The meaning of the variable has also changed. Now, any value greater than zero causes floats to be formatted in their shortest precise decimal representation. That is, the string representation produced is closer to the actual binary value than to any other value. (Previously, this was only the behavior when extra_float_digits was set to 3.) This change was made in accordance with an equivalent change that was part of the PostgreSQL 12.0 release. The behavior of a non-positive extra_float_digits value is unchanged. That will still reduce the number of float digits shown in the output string. The formula for number of digits shown is `max(1, (DIGITS + extra_float_digits))`, where DIGITS=6 for FLOAT4 values, and DIGITS=15 for FLOAT8 values. ### sql/pgwire: fix formatting of float arrays Release note (bug fix): Fixed the formatting of floats in arrays sent over the client-server pgwire protocol so that they respect the extra_float_digits parameter, and correctly format infinity values. ### eval: use common function for float->text cast There's no behavior change, but this reduces code duplication. This also adds a test that demonstrates a gap in our type/cast system. Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>

craig · 2022-05-30T04:11:46Z

Build failed:

GitHub CI (Cockroach)

rafiss · 2022-05-30T13:43:45Z

bors=otan

rafiss · 2022-05-30T17:15:18Z

bors r=otan

rafiss · 2022-05-30T19:59:51Z

bors r=otan

craig · 2022-05-30T21:45:37Z

Build succeeded:

GitHub CI (Cockroach)

This commit removes a TODO that was partially addressed by cockroachdb#82022. Informs cockroachdb#73747 Release note: None

This commit removes a TODO that was partially addressed by cockroachdb#82022. Informs cockroachdb#73743 Release note: None

105736: opt: wrap virtual computed column projections in a cast r=mgartner a=mgartner #### sql/logictest: remove assignment cast TODO This commit removes a TODO that was partially addressed by #82022. Informs #73743 Release note: None #### sql/types: fix T.Identical This commit fixes a bug in `types.T.Identical` which caused it to return false for collated string types with a different string-representation of locales that represents the same locale logically. For example, collated string types with locales `en_US` and `en_us` would not be identical, even though these are both valid representations of the same locale. There is no release note because this has not caused any known bugs. Release note: None #### opt: wrap virtual computed column projections in a cast optbuilder wraps an assignment cast around a virtual computed column expression when writing to the table if the expression does not have the same type as the virtual column. This matches the behavior of all writes to columns of expressions that don't match the target column type. However, the same cast was not applied to the projection of the virtual column expression when reading from the table. This cast is necessary in order to produce the correct value of the column - the projection must produce the same value that would have been written to the table if the column was a stored computed column. This commit corrects optbuilder so that the cast is correctly added to the projection. Note that this commit adds a standard cast, not an assignment cast, as a temporary workaround until #81698 is addressed. This is because an assignment cast can fail in cases when a regular cast will not. Because we don't validate that all existing rows can successfully produce a new virtual column expression during a backfill, adding an assignment cast to the projection of the virtual column could cause future reads to error. Once #81698 is addressed, we can change these casts to assignment casts so that they directly match the values that would be written to the same column if it were stored. Fixes #91817 Informs #81698 Release note (bug fix): A bug has been fixed that could produce incorrect values for virtual computed columns in rare cases. The bug only presented when the virtual column expression's type did not match the type of the virtual column. 105932: opt/exec: add passthrough cols to DELETE USING result cols in explain r=yuzefovich,DrewKimball a=michae2 Now that we support `DELETE USING`, delete nodes can have passthrough columns. Add these to the result columns used by `EXPLAIN (TYPES)`. Fixes: #105803 Release note (sql change): Fix an internal error when using `EXPLAIN (TYPES)` on a `DELETE FROM ... USING ... RETURNING` statement. Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com> Co-authored-by: Michael Erickson <michae2@cockroachlabs.com>

This commit removes a TODO that was partially addressed by cockroachdb#82022. Informs cockroachdb#73743 Release note: None

rafiss requested review from otan and a team May 28, 2022 19:24

rafiss requested a review from a team as a code owner May 28, 2022 19:24

knz reviewed May 28, 2022

View reviewed changes

otan approved these changes May 29, 2022

View reviewed changes

rafiss force-pushed the fmt-floats branch 4 times, most recently from 34af9ad to 9786005 Compare May 29, 2022 23:53

rafiss added 4 commits May 30, 2022 00:00

pgwire: fix formatting of float arrays and tuples

b43ed2b

Release note (bug fix): Fixed the formatting of floats in arrays and tuples sent over the client-server pgwire protocol so that they respect the extra_float_digits parameter, and correctly format infinity values.

eval: use common function for float->text cast

4d16dbf

There's no behavior change, but this reduces code duplication. This also adds a test that demonstrates a gap in our type/cast system. Release note: None

pgwire: update encoding tests to use new default

5cfe018

The encoding tests were updated so they use the new default value of 1 for extra_float_digits. This makes the test more aligned to Postgres behavior. Release note: None

rafiss force-pushed the fmt-floats branch from 9786005 to 5cfe018 Compare May 30, 2022 04:00

craig bot merged commit 9ebc6c8 into cockroachdb:master May 30, 2022

cockroach-teamcity mentioned this pull request May 30, 2022

sql/pgwire: update float formatting to be compatible with PG12 cockroachdb/docs#13986

Open

rafiss deleted the fmt-floats branch May 31, 2022 20:11

rafiss mentioned this pull request Jul 13, 2022

sql: quote_literal inconsistently truncates float4 arrays #84326

Open

mgartner added a commit to mgartner/cockroach that referenced this pull request Jun 28, 2023

sql/logictest: remove assignment cast TODO

20805cd

This commit removes a TODO that was partially addressed by cockroachdb#82022. Informs cockroachdb#73747 Release note: None

mgartner mentioned this pull request Jun 28, 2023

opt: wrap virtual computed column projections in a cast #105736

Merged

mgartner added a commit to mgartner/cockroach that referenced this pull request Jun 29, 2023

sql/logictest: remove assignment cast TODO

ab5e955

This commit removes a TODO that was partially addressed by cockroachdb#82022. Informs cockroachdb#73743 Release note: None

mgartner mentioned this pull request Jul 20, 2023

sql: truncate floats to FLOAT4 correctly #73743

Open

mgartner added a commit to mgartner/cockroach that referenced this pull request Aug 14, 2024

sql/logictest: remove assignment cast TODO

23b5d31

This commit removes a TODO that was partially addressed by cockroachdb#82022. Informs cockroachdb#73743 Release note: None

mgartner mentioned this pull request Aug 14, 2024

release-23.1: opt: wrap virtual computed column projections in a cast #129008

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql,pgwire: fixes for float formatting #82022

sql,pgwire: fixes for float formatting #82022

rafiss commented May 28, 2022

cockroach-teamcity commented May 28, 2022

knz left a comment

otan May 29, 2022

rafiss May 29, 2022

otan May 29, 2022

rafiss May 29, 2022

otan May 29, 2022

rafiss May 29, 2022

rafiss commented May 30, 2022

craig bot commented May 30, 2022

rafiss commented May 30, 2022

craig bot commented May 30, 2022

rafiss commented May 30, 2022

rafiss commented May 30, 2022

rafiss commented May 30, 2022

craig bot commented May 30, 2022

sql,pgwire: fixes for float formatting #82022

sql,pgwire: fixes for float formatting #82022

Conversation

rafiss commented May 28, 2022

sql/pgwire: update float formatting to be compatible with PG12

sql/pgwire: fix formatting of float arrays

eval: use common function for float->text cast

cockroach-teamcity commented May 28, 2022

knz left a comment

Choose a reason for hiding this comment

otan May 29, 2022

Choose a reason for hiding this comment

rafiss May 29, 2022

Choose a reason for hiding this comment

otan May 29, 2022

Choose a reason for hiding this comment

rafiss May 29, 2022

Choose a reason for hiding this comment

otan May 29, 2022

Choose a reason for hiding this comment

rafiss May 29, 2022

Choose a reason for hiding this comment

rafiss commented May 30, 2022

craig bot commented May 30, 2022

rafiss commented May 30, 2022

craig bot commented May 30, 2022

rafiss commented May 30, 2022

rafiss commented May 30, 2022

rafiss commented May 30, 2022

craig bot commented May 30, 2022