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

sql: truncate floats to FLOAT4 correctly #73743

Open
mgartner opened this issue Dec 13, 2021 · 9 comments
Open

sql: truncate floats to FLOAT4 correctly #73743

mgartner opened this issue Dec 13, 2021 · 9 comments
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. P-3 Issues/test failures with no fix SLA T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) T-sql-queries SQL Queries Team

Comments

@mgartner
Copy link
Collaborator

mgartner commented Dec 13, 2021

When inserting a float into a FLOAT4 column, or casting a value to a FLOAT4, CRDB does not correctly truncate the precision of the float to fit within a FLOAT4. This is present in v21.2.2, so it's not a regression from the assignment cast changes that were merged after v21.2, but it is related to assignment casts.

In Postgres:

marcus=# CREATE TABLE t (
  f4 FLOAT4,
  f8 FLOAT8
);
CREATE TABLE

marcus=# INSERT INTO t VALUES (18754999.99, 18754999.99);
INSERT 0 1

marcus=# SELECT * FROM t;
     f4     |     f8
------------+-------------
 1.8755e+07 | 18754999.99
(1 row)

marcus=# SELECT f8::FLOAT4 FROM t;
     f8
------------
 1.8755e+07
(1 row)

In CRDB (using logic test because the demo client will truncate floats):

statement ok
CREATE TABLE t (
  f4 FLOAT4,
  f8 FLOAT8
)

statement ok
INSERT INTO t VALUES (18754999.99, 18754999.99)

# The value of f4 should be 1.8755e+07.
query RR colnames
SELECT * FROM t
----
f4               f8
1.875499999e+07  1.875499999e+07

# The result should be 1.8755e+07.
query R
SELECT f8::FLOAT4 FROM t
----
1.875499999e+07

Jira issue: CRDB-11718

@mgartner mgartner added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Dec 13, 2021
@blathers-crl blathers-crl bot added the T-sql-queries SQL Queries Team label Dec 13, 2021
@mgartner
Copy link
Collaborator Author

The issue can be observed with explicit casts in the demo. In the example below, the result value should be 18755000.

defaultdb> SELECT 18754999.99::FLOAT8::FLOAT4::FLOAT8;
      float8
-------------------
  1.875499999e+07
(1 row)

@mgartner mgartner self-assigned this Dec 13, 2021
mgartner added a commit to mgartner/cockroach that referenced this issue Dec 14, 2021
The `EliminateAssignmentCast` rule has been combined with the
`EliminateCast` rule. Now an assignment cast is eliminated if the source
and target type are identical. This now possible thanks to some changes
to type resolution, including:

  1. Placeholder types are resolved with unspecified type modifiers.
     This ensures that assignment casts won't be eliminated if the a
     placeholder value does not conform to the target type's modifiers.

  2. When constant integer `NumVal`s are resolved as a typed-value,
     they are validated to ensure they fit within their type's width.
     There may be more types we need to perform similar validation for,
     such as floats (see cockroachdb#73743).

  3. Columns in values expressions with values that have non-identical
     types but the same type OID will be typed with type modifiers. For
     example, if a values expression has a CHAR(1) value and a CHAR(3)
     value in the same column, the column will be typed as a CHAR
     without an explicit width.

Fixes cockroachdb#73450

Release note (bug fix): A bug has been fixed that caused incorrect
evaluation of placeholder values in EXECUTE statements. The bug
presented when the PREPARE statement cast a placeholder value, for
example `PREPARE s AS SELECT $1::INT2`. If the assigned value for `$1`
exceeded the maximum width value of the cast target type, the result
value of the cast could be incorrect. This bug has been present since
version 19.1 or earlier.
mgartner added a commit to mgartner/cockroach that referenced this issue Dec 16, 2021
The `EliminateAssignmentCast` rule has been combined with the
`EliminateCast` rule. Now an assignment cast is eliminated if the source
and target type are identical. This now possible thanks to some changes
to type resolution, including:

  1. Placeholder types are resolved with unspecified type modifiers.
     This ensures that assignment casts won't be eliminated if the a
     placeholder value does not conform to the target type's modifiers.

  2. When constant integer `NumVal`s are resolved as a typed-value,
     they are validated to ensure they fit within their type's width.
     There may be more types we need to perform similar validation for,
     such as floats (see cockroachdb#73743).

  3. Columns in values expressions with values that have non-identical
     types but the same type OID will be typed with type modifiers. For
     example, if a values expression has a CHAR(1) value and a CHAR(3)
     value in the same column, the column will be typed as a CHAR
     without an explicit width.

  4. Type modifiers are now correctly omitted from array content types
     when arrays contain constants.

Fixes cockroachdb#73450

Release note (bug fix): A bug has been fixed that caused incorrect
evaluation of placeholder values in EXECUTE statements. The bug
presented when the PREPARE statement cast a placeholder value, for
example `PREPARE s AS SELECT $1::INT2`. If the assigned value for `$1`
exceeded the maximum width value of the cast target type, the result
value of the cast could be incorrect. This bug has been present since
version 19.1 or earlier.
mgartner added a commit to mgartner/cockroach that referenced this issue Dec 16, 2021
The `EliminateAssignmentCast` rule has been combined with the
`EliminateCast` rule. Now an assignment cast is eliminated if the source
and target type are identical. This now possible thanks to some changes
to type resolution, including:

  1. Placeholder types are resolved with unspecified type modifiers.
     This ensures that assignment casts won't be eliminated if the a
     placeholder value does not conform to the target type's modifiers.

  2. When constant integer `NumVal`s are resolved as a typed-value,
     they are validated to ensure they fit within their type's width.
     There may be more types we need to perform similar validation for,
     such as floats (see cockroachdb#73743).

  3. Columns in values expressions with values that have non-identical
     types but the same type OID will be typed with type modifiers. For
     example, if a values expression has a CHAR(1) value and a CHAR(3)
     value in the same column, the column will be typed as a CHAR
     without an explicit width.

  4. Type modifiers are now correctly omitted from array content types
     when arrays contain constants.

Fixes cockroachdb#73450

Release note (bug fix): A bug has been fixed that caused incorrect
evaluation of placeholder values in EXECUTE statements. The bug
presented when the PREPARE statement cast a placeholder value, for
example `PREPARE s AS SELECT $1::INT2`. If the assigned value for `$1`
exceeded the maximum width value of the cast target type, the result
value of the cast could be incorrect. This bug has been present since
version 19.1 or earlier.
craig bot pushed a commit that referenced this issue Dec 17, 2021
73762: opt: eliminate assignment casts with identical source and target types r=mgartner a=mgartner

#### sql: remove type modifiers from placeholder types

The width and precision of placeholder values are not known during
PREPARE, therefore we remove any type modifiers from placeholder types
during type checking so that a value of any width will fit within the
palceholder type.

Note that this change is similar to the changes made in #70722 to
placeholder type checking that were later reverted in #72793. In #70722
the type OIDs of placeholders could be altered, e.g., a placeholder
originally with type `INT2` would be converted to an `INT`. In this
commit, type OIDs of placeholders are not changing, only type modifiers
are.

This commit should allow some logic added in #72793 to be simplified or
removed entirely.

Release note: None

#### opt: eliminate assignment casts with identical source and target types

The `EliminateAssignmentCast` rule has been combined with the
`EliminateCast` rule. Now an assignment cast is eliminated if the source
and target type are identical. This now possible thanks to some changes
to type resolution, including:

  1. Placeholder types are resolved with unspecified type modifiers.
     This ensures that assignment casts won't be eliminated if the a
     placeholder value does not conform to the target type's modifiers.

  2. When constant integer `NumVal`s are resolved as a typed-value,
     they are validated to ensure they fit within their type's width.
     There may be more types we need to perform similar validation for,
     such as floats (see #73743).

  3. Columns in values expressions with values that have non-identical
     types but the same type OID will be typed with type modifiers. For
     example, if a values expression has a CHAR(1) value and a CHAR(3)
     value in the same column, the column will be typed as a CHAR
     without an explicit width.

  4. Type modifiers are now correctly omitted from array content types
     when arrays contain constants.

Fixes #73450

Release note (bug fix): A bug has been fixed that caused incorrect
evaluation of placeholder values in EXECUTE statements. The bug
presented when the PREPARE statement cast a placeholder value, for
example `PREPARE s AS SELECT $1::INT2`. If the assigned value for `$1`
exceeded the maximum width value of the cast target type, the result
value of the cast could be incorrect. This bug has been present since
version 19.1 or earlier.


73819: bazel: upgrade `rules_foreign_cc` to 0.7 r=rail a=rickystewart

Also add `-fcommon` to compile flags for `krb5`.

Closes #71306.

Release note: None

73832: sql/opt/exec: output index/expiry in EXPLAIN SPLIT/RELOCATE statements r=RaduBerinde a=knz

Release note (sql change): The output of `EXPLAIN ALTER INDEX/TABLE
... RELOCATE/SPLIT` now includes the target table/index name and, for
the SPLIT AT variants, the expiry timestamp.

Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>
Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
gustasva pushed a commit to gustasva/cockroach that referenced this issue Jan 4, 2022
The `EliminateAssignmentCast` rule has been combined with the
`EliminateCast` rule. Now an assignment cast is eliminated if the source
and target type are identical. This now possible thanks to some changes
to type resolution, including:

  1. Placeholder types are resolved with unspecified type modifiers.
     This ensures that assignment casts won't be eliminated if the a
     placeholder value does not conform to the target type's modifiers.

  2. When constant integer `NumVal`s are resolved as a typed-value,
     they are validated to ensure they fit within their type's width.
     There may be more types we need to perform similar validation for,
     such as floats (see cockroachdb#73743).

  3. Columns in values expressions with values that have non-identical
     types but the same type OID will be typed with type modifiers. For
     example, if a values expression has a CHAR(1) value and a CHAR(3)
     value in the same column, the column will be typed as a CHAR
     without an explicit width.

  4. Type modifiers are now correctly omitted from array content types
     when arrays contain constants.

Fixes cockroachdb#73450

Release note (bug fix): A bug has been fixed that caused incorrect
evaluation of placeholder values in EXECUTE statements. The bug
presented when the PREPARE statement cast a placeholder value, for
example `PREPARE s AS SELECT $1::INT2`. If the assigned value for `$1`
exceeded the maximum width value of the cast target type, the result
value of the cast could be incorrect. This bug has been present since
version 19.1 or earlier.
@blathers-crl blathers-crl bot added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Jan 18, 2022
@mgartner mgartner removed their assignment Jan 20, 2022
@otan
Copy link
Contributor

otan commented Feb 9, 2022

root cause is float to float casts do nothing:

case *DFloat:
return d, nil

for int family, it is done here...

func AdjustValueToType(typ *types.T, inVal Datum) (outVal Datum, err error) {

@otan
Copy link
Contributor

otan commented Feb 9, 2022

float4 is actually handled pretty badly.

parsing example:

root@127.0.0.1:26257/defaultdb> select '4e+38'::float4;
   float4
------------
  Infinity
(1 row)

vs psql:

otan=# select '4e+38'::float4;
ERROR:  "4e+38" is out of range for type real
LINE 1: select '4e+38'::float4;

@otan
Copy link
Contributor

otan commented Feb 9, 2022

urgh, this is already committed into the DB for some people as well. would need to test whether any fix breaks those cases...

@otan
Copy link
Contributor

otan commented Feb 9, 2022

in fact, since we store Float4 as Float8, all arithmetic is done using Float8 so even more stuff is probably wrong?

@mgartner
Copy link
Collaborator Author

mgartner commented Feb 9, 2022

in fact, since we store Float4 as Float8, all arithmetic is done using Float8 so even more stuff is probably wrong?

Yikes, I think you're probably correct about that. Looks like this won't be a quick win...

@rafiss
Copy link
Collaborator

rafiss commented Feb 7, 2023

The root problem here seems to be #48613

@exalate-issue-sync exalate-issue-sync bot removed the T-sql-queries SQL Queries Team label Jun 1, 2023
mgartner added a commit to mgartner/cockroach that referenced this issue Jun 29, 2023
This commit removes a TODO that was partially addressed by cockroachdb#82022.

Informs cockroachdb#73743

Release note: None
craig bot pushed a commit that referenced this issue Jul 5, 2023
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>
@mgartner
Copy link
Collaborator Author

It looks like this was partially fixed by #82022. See: 4d16dbf#diff-c49e1bdbc0004860cadef9d5e1e4d3e2c18b89a254ad9f4885e6c929ff2a5e43R172-R173

But it's not clear to me how that change fixed these tests and the example test in the description above. Maybe @rafiss has an idea?

Regardless, I think this is still an issue as seen here: 4d16dbf#diff-c49e1bdbc0004860cadef9d5e1e4d3e2c18b89a254ad9f4885e6c929ff2a5e43R1405-R1411

@rafiss
Copy link
Collaborator

rafiss commented Jul 20, 2023

Another related issue: #84326

@mgartner mgartner moved this to New Backlog in SQL Queries Jul 24, 2023
@yuzefovich yuzefovich added the T-sql-queries SQL Queries Team label May 2, 2024
@mgartner mgartner moved this from Backlog to Bugs to Fix in SQL Queries Aug 14, 2024
@mgartner mgartner added the P-3 Issues/test failures with no fix SLA label Aug 14, 2024
mgartner added a commit to mgartner/cockroach that referenced this issue Aug 14, 2024
This commit removes a TODO that was partially addressed by cockroachdb#82022.

Informs cockroachdb#73743

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. P-3 Issues/test failures with no fix SLA T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) T-sql-queries SQL Queries Team
Projects
Status: Bugs to Fix
Development

Successfully merging a pull request may close this issue.

4 participants