Skip to content

Conversation

@dils2k
Copy link
Contributor

@dils2k dils2k commented Dec 3, 2025

Decimal decoding implementation was ignoring trailing zeros after the decimal point that were implicitly added by the dscale value, resulting precision losses. For example, 1.23000 could be decoded as 1.23. This commit fixes it by adding missing zeros.

Release note: None
Epic: CRDB-57092
Fixes: #158093

@dils2k dils2k requested review from a team as code owners December 3, 2025 20:31
@blathers-crl
Copy link

blathers-crl bot commented Dec 3, 2025

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

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.

@blathers-crl blathers-crl bot added the O-community Originated from the community label Dec 3, 2025
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@dils2k dils2k marked this pull request as draft December 3, 2025 20:35
Decimal decoding implementation was ignoring trailing
zeros after the decimal point that were implicitly added
by the dscale value, resulting precision losses. For
example, 1.23000 could be decoded as 1.23. This commit
fixes it by adding missing zeros.

Release note: None
Epic: CRDB-57092
Fixes: cockroachdb#158093
@dils2k dils2k force-pushed the fix-binary-decimal branch from 661c11b to 1598624 Compare December 4, 2025 02:25
@blathers-crl
Copy link

blathers-crl bot commented Dec 4, 2025

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.

Copy link
Collaborator

@DrewKimball DrewKimball 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 @dils2k)


pkg/sql/pgwire/pgwirebase/encoding.go line 656 at r1 (raw file):

				// * if the digits we have in the buffer on the RHS, as calculated above,
				// is larger than our calculated dscale, truncate our buffer to match the
				// desired dscale.

Let's make sure to update this comment to describe the case when the buffer is padded with 0's and give an example as well.


pkg/sql/pgwire/pgwirebase/encoding.go line 657 at r1 (raw file):

				// is larger than our calculated dscale, truncate our buffer to match the
				// desired dscale.
				dscale := (alloc.pgNum.Ndigits - (alloc.pgNum.Weight + 1)) * PGDecDigits

I think we should go a bit further with this change. This code block is basically just modifying dscale to be equal to alloc.pgNum.Dscale, and padding or truncating the buffer to compensate for too many or too few trailing zeros in the logic above.

We should get rid of the logic that modifies dscale in this block, and just use alloc.pgNum.Dscale below when setting the decimal's exponent. dscale (which we could rename to curDScale or something) will then only be used to determine how much to pad or truncate the buffer.


pkg/sql/pgwire/testdata/pgtest/decimal line 48 at r1 (raw file):

# CockroachDB intentionally uses exponents for decimals like 1E+4, as
# oppposed to Postgres, which returns 10000.
until crdb_only

I expect that we'll no longer differ from postgres for this case. You can get rid of this test case entirely, and remove the noncrdb_only flag from the case below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

O-community Originated from the community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sql/pgwire: decimal loses precision when inserted with binary format

3 participants