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

tree: fix decimal strings for 0 with exponents < -6 #102299

Merged
merged 1 commit into from
May 18, 2023

Conversation

otan
Copy link
Contributor

@otan otan commented Apr 26, 2023

Release note (bug fix): Fix a bug where 0 with exponents < -6 would display
as 0E(exponent) instead of printing all 0s, e.g. 0E-7 should be 0.0000000.

Informs #102217

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rafiss
Copy link
Collaborator

rafiss commented May 9, 2023

@otan thanks for picking this up! are there any risks here, or should we just go for this approach?

@otan
Copy link
Contributor Author

otan commented May 9, 2023

i'm fine with it if you are ;)

@otan
Copy link
Contributor Author

otan commented May 9, 2023

(but we'll want to merge cockroachdb/apd#128 first)

@otan otan marked this pull request as ready for review May 15, 2023 01:11
@otan otan requested a review from a team as a code owner May 15, 2023 01:11
@otan otan added the backport-23.1.x Flags PRs that need to be backported to 23.1 label May 15, 2023
@otan otan force-pushed the dd branch 2 times, most recently from 0d8cfab to 87c279f Compare May 15, 2023 03:42
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

does this fix #102217 or are there more cases to deal with?

i'm surprised the ::decimal cases in pkg/cmd/generate-binary/main.go weren't affected. could we add a a test there that shows the fix in action?

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


pkg/sql/logictest/testdata/logic_test/decimal line 480 at r1 (raw file):

0          -1         -0                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                      0     -0
0          4.2        0E+1                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                    0.0   0
0          Infinity   0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000   0     0

i tried this query on postgres and got this result for this row:

         0 |  Infinity |                       0 |    0 |         0

Release note (bug fix): Fix a bug where 0 with exponents < -6 would display
as 0E(exponent) instead of printing all 0s, e.g. 0E-7 should be 0.0000000.
Copy link
Contributor Author

@otan otan left a comment

Choose a reason for hiding this comment

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

does this fix #102217 or are there more cases to deal with?

this fixes the display issue, yeah

i'm surprised the ::decimal cases in pkg/cmd/generate-binary/main.go weren't affected. could we add a a test there that shows the fix in action?

yep

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


pkg/sql/logictest/testdata/logic_test/decimal line 480 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i tried this query on postgres and got this result for this row:

         0 |  Infinity |                       0 |    0 |         0

this is a bug in our implementation more than anything, and is unrelated to this PR.

pg:

otan=# select 0::decimal / 'infinity'::decimal;
 ?column?
----------
        0
(1 row)

crdb, before this pr:

root@:26257/defaultdb> select 0::decimal / 'infinity'::decimal;
  ?column?
------------
  0E-2019
(1 row)

i'm guessing our use of HighPrecisionContext to divide Infinity is incorrect (it's also incorrect for all the other cases). Seems outside the scope of what I need out of this PR though.

@otan otan requested a review from rafiss May 15, 2023 22:43
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

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


pkg/sql/logictest/testdata/logic_test/decimal line 480 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

this is a bug in our implementation more than anything, and is unrelated to this PR.

pg:

otan=# select 0::decimal / 'infinity'::decimal;
 ?column?
----------
        0
(1 row)

crdb, before this pr:

root@:26257/defaultdb> select 0::decimal / 'infinity'::decimal;
  ?column?
------------
  0E-2019
(1 row)

i'm guessing our use of HighPrecisionContext to divide Infinity is incorrect (it's also incorrect for all the other cases). Seems outside the scope of what I need out of this PR though.

hm ok - but do you have a sense of what is needed in apd to make 0.00000...0 display as 0? would be nice to address later

@otan
Copy link
Contributor Author

otan commented May 18, 2023

pkg/sql/logictest/testdata/logic_test/decimal line 480 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

hm ok - but do you have a sense of what is needed in apd to make 0.00000...0 display as 0? would be nice to address later

it's not apd we need to change. it's all the execution stuff that makes 0 / infinity not use HighScalePrecision to divide.

@otan
Copy link
Contributor Author

otan commented May 18, 2023

pkg/sql/logictest/testdata/logic_test/decimal line 480 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

it's not apd we need to change. it's all the execution stuff that makes 0 / infinity not use HighScalePrecision to divide.

(we need to change the builtin or whatever that does this)

@otan
Copy link
Contributor Author

otan commented May 18, 2023

bors r=rafiss

@otan
Copy link
Contributor Author

otan commented May 18, 2023

filed #102299

@otan
Copy link
Contributor Author

otan commented May 18, 2023

pkg/sql/logictest/testdata/logic_test/decimal line 480 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

it's not apd we need to change. it's all the execution stuff that makes 0 / infinity not use HighScalePrecision to divide.

actually it may be Divide in apd, stuff like 0::decimal / 5 performs correctly.

@craig
Copy link
Contributor

craig bot commented May 18, 2023

Build succeeded:

@craig craig bot merged commit b62c5f7 into cockroachdb:master May 18, 2023
@blathers-crl
Copy link

blathers-crl bot commented May 18, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error setting reviewers, but backport branch blathers/backport-release-23.1-102299 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/103640/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. []

Backport to branch 23.1.x failed. See errors above.


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

andyyang890 added a commit to andyyang890/activerecord-cockroachdb-adapter that referenced this pull request May 22, 2023
This patch fixes the default numbers test to expect the correct result
after cockroachdb/cockroach#102299 was merged.
andyyang890 added a commit to andyyang890/activerecord-cockroachdb-adapter that referenced this pull request May 22, 2023
This patch fixes the default numbers test to expect the correct result
after cockroachdb/cockroach#102299 was merged.
andyyang890 added a commit to andyyang890/activerecord-cockroachdb-adapter that referenced this pull request May 23, 2023
This patch fixes the default numbers test to expect the correct result
after cockroachdb/cockroach#102299 was merged.
andyyang890 added a commit to andyyang890/activerecord-cockroachdb-adapter that referenced this pull request May 23, 2023
This patch fixes the default numbers test to expect the correct result
after cockroachdb/cockroach#102299 was merged.
andyyang890 added a commit to andyyang890/activerecord-cockroachdb-adapter that referenced this pull request May 26, 2023
This patch fixes the default numbers test to expect the correct result
after cockroachdb/cockroach#102299 was merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants