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

format: format 0E-x where x > 6 like PostgreSQL does #128

Merged
merged 1 commit into from
May 15, 2023

Conversation

otan
Copy link
Contributor

@otan otan commented Apr 25, 2023

Officially in the GDA spec, formatting a 0 with any decimal places should be 0. However, this code doesn't do that - it formats the number of 0s up to 6, and then 0E-exponent afterwards.

To make this more "PG" compatible, we fix this by ensuring we display as many 0s as the exponent allows in the .String() case.

Refs cockroachdb/cockroach#102217, but not 100% sure I like the solution.

Copy link

@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.

lgtm! but nit: since this is a lower level library used by others, i wonder if we should make the commit/comments say "PostgreSQL" instead of PG. ideally we'd not have to mention PG at all, but it sounds like we have to, since PG deviates from the official spec?

@otan otan changed the title format: format 0E-x where x > 6 like PG does format: format 0E-x where x > 6 like PostgreSQL does May 15, 2023
@otan
Copy link
Contributor Author

otan commented May 15, 2023

yep, done

Officially in the GDA spec, formatting a 0 with any decimal places
should be 0. However, this code doesn't do that - it formats the number
of 0s up to 6, and then `0E-exponent` afterwards.

To make this more "PG" compatible, we fix this by ensuring we display as
many 0s as the exponent allows in the `.String()` case.
@otan otan merged commit 4c2545f into cockroachdb:master May 15, 2023
@otan otan deleted the decimal_fix branch May 15, 2023 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants