Skip to content

fix(arrow/decimal): widen Decimal64 rescale to 64 bits - #1307

Open
winklemad wants to merge 1 commit into
apache:mainfrom
winklemad:fix-decimal64-rescale-truncation
Open

fix(arrow/decimal): widen Decimal64 rescale to 64 bits#1307
winklemad wants to merge 1 commit into
apache:mainfrom
winklemad:fix-decimal64-rescale-truncation

Conversation

@winklemad

Copy link
Copy Markdown

Rationale for this change

Decimal64.rescaleWouldCauseDataLoss (arrow/decimal/decimal.go) is a copy of the Decimal32 version that was never widened to 64 bits — it still uses bits.Div32 / bits.Mul32 with uint32(n) / uint32(multiplier). Any Decimal64 whose magnitude exceeds uint32 max (~4.29e9, well inside Decimal64's 18-digit range) is truncated to 32 bits before the multiply/divide, so Rescale returns a corrupt value and/or a spurious rescale data loss error.

Concretely, on main:

decimal.Decimal64(10_000_000_000).Rescale(2, 1)
// returns (141006540, error "rescale data loss")
// correct: (1000000000, nil)   // 1e10 / 10 = 1e9, exact

What changes are included in this PR?

  • Use bits.Div64 / bits.Mul64 over uint64 in Decimal64.rescaleWouldCauseDataLoss (the Decimal32 version is already correct for 32 bits).
  • TestDecimalRescale had baked in the truncated behavior: it asserted that Decimal64(555555).Rescale(0, 5) returns rescale data loss, but 555555 at scale 5 is 55,555,500,000, which fits in Decimal64 and is lossless. Corrected that assertion, checked the exact result, and added a value that genuinely overflows Decimal64 (5e18 rescaled up) so the real data-loss path stays covered.

RED→GREEN verified: the updated TestDecimalRescale fails on the unpatched code and passes with the fix; the full arrow/decimal package suite is green; gofmt/go vet/golangci-lint clean.

Are these changes tested?

Yes — arrow/decimal/decimal_test.go.

Are there any user-facing changes?

Decimal64.Rescale now returns correct results (and no spurious error) for values above ~4.29e9. No API change.

Decimal64.rescaleWouldCauseDataLoss was a copy of the Decimal32 version
and still used bits.Div32/bits.Mul32 with uint32(n)/uint32(multiplier),
truncating any Decimal64 value above uint32 max (~4.29e9) to 32 bits.
Rescale then returned a corrupt value and/or a spurious "rescale data
loss" error for values that are well within Decimal64's range, e.g.
Decimal64(1e10).Rescale(2, 1) returned 141006540 with an error instead
of 1000000000.

Use bits.Div64/bits.Mul64 over uint64. The existing test asserted the
truncated behavior for Decimal64; corrected it and added a genuine
overflow case.

Signed-off-by: Madan Kumar <winklemad@outlook.com>
@winklemad
winklemad requested a review from zeroshade as a code owner September 9, 2026 05:02
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.

1 participant