Skip to content

sql: match precision of decimal operations to other databases#168724

Draft
bghal wants to merge 1 commit intocockroachdb:masterfrom
bghal:sql-decimal-precision
Draft

sql: match precision of decimal operations to other databases#168724
bghal wants to merge 1 commit intocockroachdb:masterfrom
bghal:sql-decimal-precision

Conversation

@bghal
Copy link
Copy Markdown
Contributor

@bghal bghal commented Apr 20, 2026

The lower precision resulted in errors in
intermediary calculations. This changes moves the
precision to 38, which matches the precision of
Oracle and SQL Server which, besides DB2, have the
least precision of the reference databases.

Fixes: #168373

Release note (sql change): Decimal expressions
evaluate with a higher precision which prevents
errors in intermediary calculations.

@trunk-io
Copy link
Copy Markdown
Contributor

trunk-io Bot commented Apr 20, 2026

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Apr 20, 2026

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

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

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@bghal bghal force-pushed the sql-decimal-precision branch 2 times, most recently from 672c69d to 2258804 Compare April 21, 2026 17:31
The lower precision resulted in errors in
intermediary calculations. This changes moves the
precision to 38, which matches the precision of
Oracle and SQL Server which, besides DB2, have the
least precision of the reference databases.

Fixes: cockroachdb#168373

Release note (sql change): Decimal expressions
evaluate with a higher precision which prevents
errors in intermediary calculations.
@bghal bghal force-pushed the sql-decimal-precision branch from 2258804 to 0a6d5ee Compare April 22, 2026 17:47
@bghal bghal marked this pull request as ready for review April 22, 2026 17:48
@bghal bghal requested review from a team as code owners April 22, 2026 17:48
@bghal bghal requested review from mw5h and removed request for a team April 22, 2026 17:48
@rafiss
Copy link
Copy Markdown
Collaborator

rafiss commented Apr 22, 2026

Thanks for looking into this issue. There are a few things that are worth discussing before going with this.

CockroachDB targets PostgreSQL compatibility as our main reference, not Oracle/SQL Server. When picking a precision, the question to ask first is "what does PG do?" rather than averaging across reference databases.

Today's precision-20 default already aligns very closely with PG. I tested a bit and looked at the code, and PG doesn't actually use a fixed precision. It dynamically selects a result scale targeting at least 16 significant digits via select_div_scale (see src/backend/utils/adt/numeric.c, NUMERIC_MIN_SIG_DIGITS = 16). I ran a few queries side-by-side against PG and current CRDB master:

Expression CRDB master PG
1::numeric/3 0.33333333333333333333 0.33333333333333333333
2::numeric/3 0.66666666666666666667 0.66666666666666666667
1::numeric/7 0.14285714285714285714 0.14285714285714285714
(1::numeric/3)*3 0.99999999999999999999 0.99999999999999999999
10::numeric/3 3.3333333333333333333 3.3333333333333333

Most cases are byte-identical. After this PR they'd all jump to 38 digits, which would be quite a meaningful divergence from PG. We would give up on some PG parity that CRDB already has.

There also could be a performance cost to consider. DecimalCtx is the global context for nearly every decimal operation including aggregates, casts, window functions, scalar arithmetic, and constant folding in the optimizer. Bumping the precision affects:

  • CPU: 38-digit coefficients no longer fit in big.Int's tiny-int fast path (20 digits ≈ 67 bits, 38 digits ≈ 127 bits), so most ops do more work. Quo cost roughly doubles; Pow/Ln/Sqrt are worse.
  • Memory: each apd.Decimal carries a larger backing array, and IntermediateCtx jumps from 25 → 43, which means more bytes shipped between nodes during distributed aggregation.

Could you run the existing micro-benchmarks (BenchmarkAvgAggregateDecimal, BenchmarkSumAggregateDecimal, BenchmarkVarianceAggregateDecimal in pkg/sql/sem/builtins/aggregate_builtins_test.go, and the colexec equivalents) before/after to quantify the impact? Bumping a global default pays the cost on every decimal op cluster-wide, so it's worth knowing the magnitude.

This is a pretty big change with lots of observable effects, so it deserves some consensus between Product and various Engineering teams. The underlying bug in #168373 is real, but a more targeted fix (e.g. only bumping precision for the specific aggregation paths that need more headroom) might be the right approach rather than a global default change.

Happy to discuss more offline.

@bghal bghal marked this pull request as draft April 24, 2026 16:27
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.

Incorrect aggregation result for variance-equivalent expression on constant input (returns -1/n instead of 0)

3 participants