Skip to content

fix: widen decimal arithmetic to Decimal256 - #28629

Open
daviszhen wants to merge 21 commits into
matrixorigin:mainfrom
daviszhen:codex/decimal-arithmetic-widen
Open

fix: widen decimal arithmetic to Decimal256#28629
daviszhen wants to merge 21 commits into
matrixorigin:mainfrom
daviszhen:codex/decimal-arithmetic-widen

Conversation

@daviszhen

@daviszhen daviszhen commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

Fixes #28584

What this PR does / why we need it:

  • Promotes Decimal128 addition, subtraction, and multiplication to Decimal256 when the declared result requires more than 38 digits, including mixed Decimal64/Decimal128 and signed/unsigned integer operands.
  • Preserves explicit DECIMAL cast domains and canonicalizes genuinely weak decimal literals, including scientific notation, before overload selection.
  • Enforces the public 65-digit DECIMAL result limit at runtime so wider Decimal256 coefficients cannot escape under DECIMAL(65) metadata.
  • Prevents comparison optimization from narrowing a wide Decimal256 expression back to Decimal128 and keeps affine SUM rewrites compatible with widened arithmetic.
  • Adds 65-digit success and 66-digit overflow boundaries, comparison and CTAS coverage, mixed-domain resolver coverage, and updates affected decimal BVT expectations.

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@aptend aptend left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Deep review of exact head ac9cee1 (merge-base 9a07655). I read the linked issue, the only issue comment, and checked all review/inline/thread history (there are no prior reviews or threads), then traced the complete six-file diff through overload selection, binder-inserted casts, Decimal128/256 kernels, comparison narrowing, affine SUM rewriting, and downstream published result types. The same-type 39/40-digit cases are correctly widened and the comparison/affine adaptations are directionally sound, but three uncovered boundaries remain blocking: mixed numeric operands still stay on Decimal128, explicit casted string literals can generate an invalid width<scale implicit cast, and products beyond the public 65-digit precision can escape inside a value tagged DECIMAL(65). Native dependencies were built. Full exact-head ./pkg/sql/plan and ./pkg/sql/plan/function tests pass, as do the changed focused tests under -race. Three review-only counterexamples reproduced the findings and were removed; git diff --check is clean.

Comment thread pkg/sql/plan/function/list_operator.go Outdated
Comment thread pkg/sql/plan/base_binder.go Outdated
Comment thread pkg/sql/plan/function/list_operator.go
@daviszhen

Copy link
Copy Markdown
Contributor Author

Resolved the latest failing checks at head d8342a4199 and merged current upstream main through b349fea56f.

The diagnostics show no Decimal regression: coverage and both Compose/Standalone BVT jobs passed. The only failed producer was ordinary Ubuntu UT:

  • pkg/tests/issues/TestWWConflict created its 10-second context before shared-cluster database/table setup;
  • under race/CI load, setup consumed that deadline and txn1 returned context deadline exceeded;
  • txn2 then waited forever on txn2StartedC, so the package reached its 40-minute timeout and the remaining embedded stage was terminated by the overall job deadline.

Fix:

  • start a 30-second context only after fixture setup, so it bounds the concurrent transaction workflow itself;
  • make waits for txn2StartedC and txn2CommittedC context-aware, preventing a failed transaction from leaving its peer blocked forever.

Local validation passed:

  • go test -tags matrixone_test -race ./pkg/tests/issues -run '^TestWWConflict$' -count=1 -timeout 10m (15.989s);
  • go test ./pkg/sql/plan ./pkg/sql/plan/function ./pkg/sql/plan/substrait -count=1;
  • make build;
  • git diff --check.

A fresh CI run has been triggered by the push.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed the full current change at d8342a4 against main b349fea, linked issue #28584, and all earlier review findings/replies. No remaining concrete blocker found.

All three previous findings are closed: widening is derived from original decimal/signed/unsigned domains after coercion; syntax-explicit CAST remains a domain boundary and weak literals are canonicalized at the declared scale; Decimal256 +, -, and * now validate non-NULL result coefficients against the published precision. The literal lookup refinement is materialized as an actual argument cast, avoiding a dispatch-only type change. Comparison narrowing now checks integral capacity as well as scale.

Traced binder/result metadata, existing Decimal256 kernels, constant/vector/NULL/selection-mask handling, casts and CTAS consumers, and the affine SUM proof/cast-back path. Existing physical operator IDs and Decimal256 representation are reused, rather than changing a legacy overload's physical argument/result contract. Affine restoration remains behind its exact integer-range/cardinality proof. Wider plans deliberately decline Substrait export instead of silently truncating to its 38-digit domain; this is a real offload/performance tradeoff explicitly recorded by the changed tests. Narrow fitting paths remain available; widened execution pays bounded 32-byte vector/cast work and one precision-validation pass.

The accompanying TestWWConflict adjustment starts its budget after setup, bounds both channel waits, retains transaction error assertions, joins both workers, and preserves deferred hook restoration. It does not retry away an assertion failure or remove the conflict scenario.

Validation: source/diff and consumer reasoning; reviewed mixed-domain resolver, explicit-cast/exponent, 65/66-digit runtime, comparison/CTAS, affine and BVT oracles. Reused the author's current-head reported plan/function/substrait package, targeted race TestWWConflict and build results, plus earlier relevant decimal BVT evidence. No fresh native suite, SQL deployment, mixed-version run or benchmark was executed by this reviewer; CI was not awaited. The PR body's older failed-build fragments are not counted as passing evidence. Historical executors still retain their own pre-fix precision-error behavior until upgraded; no new wire/disk representation is introduced.

@daviszhen

Copy link
Copy Markdown
Contributor Author

已确认 aptend 提出的三个 Decimal256 精度边界均已落实:mixed numeric coercion 后 widening、显式 DECIMAL cast/弱字面量 domain 处理、以及 Decimal256 结果 65 位精度校验;对应 threads 均已解决,XuPeng-SH 已在上一 head 批准。

本次已同步当前 upstream/main(23b20ea909),新 head 为 a2fb9b1

验证:

  • go test ./pkg/sql/plan ./pkg/sql/plan/function ./pkg/sql/plan/substrait -count=1:通过
  • make build:通过
  • git diff --check:通过

请复审当前 head。

@daviszhen

Copy link
Copy Markdown
Contributor Author

Synced the latest main at head 62c370f after main advanced during review. The Decimal256 precision constraints remain intact. Revalidated go test ./pkg/sql/plan ./pkg/sql/plan/function ./pkg/sql/plan/substrait -count=1 and make build; all pass.

…metic-widen

# Conflicts:
#	pkg/sql/plan/function/list_operator.go
@daviszhen

Copy link
Copy Markdown
Contributor Author

Synced with the latest upstream main (6602ca0) and resolved the arithmetic cast-rule conflict.\n\nConflict resolution:\n- preserve main's arithmeticTypeCastRule1 behavior for BIT unsigned range\n- then apply widenedDecimalArithmeticInputs for DECIMAL +, -, and *\n\nLocal verification:\n- go test ./pkg/sql/plan ./pkg/sql/plan/function ./pkg/sql/plan/substrait -count=1\n- make build\n\nAll passed. The branch is no longer behind main and GitHub reports it as mergeable. New head: 3383f8c.

@daviszhen

Copy link
Copy Markdown
Contributor Author

Fixed the failing PESSIMISTIC Compose BVT check in 2eebaf00ab.

Root cause:

  • The failure was in test/distributed/cases/dtype/bit.sql.
  • The golden result still expected Decimal128 Mul overflow for (2^64 - 1)^2.
  • This PR intentionally widens the expression to Decimal256, so the actual exact value 340282366920938463426481119284349108225 is the correct new behavior.

Changes:

  • Updated the BIT BVT SQL/result to assert the widened Decimal256 result.
  • Synchronized the branch with the latest main.

Local verification:

  • go test ./pkg/sql/plan ./pkg/sql/plan/function ./pkg/sql/plan/substrait -count=1 passed.
  • make build passed.
  • git diff --check passed.

A new CI run has been triggered for head 2eebaf00ab5ef2aa720f68fc278e847af37f58e7. Docker is not installed on this local machine, so the exact Compose BVT will be verified by CI.

@daviszhen

Copy link
Copy Markdown
Contributor Author

Follow-up fix pushed in bea96bede2.

The PROXY Compose NumberFormatException was caused by the explanatory SQL comments added between two result blocks. mo-tester appended those comments to the preceding numeric cell, then attempted to parse this combined text:

17878103347812890625 + comment text

The Decimal256 value itself was not the parse failure. I removed the inter-query comments while retaining the exact widened result assertion.

The other Ubuntu UT failure was TestISCPExecutor1: its watermark stayed at 0-0 during the CI wait. This is unrelated to the decimal code path, and the focused race-detector test passes locally:

go test -tags matrixone_test -race ./pkg/vm/engine/test -run ^TestISCPExecutor1$ -count=1 -timeout 10m

Also verified after synchronizing latest main:

  • decimal/plan test packages pass
  • make build passes
  • git diff --check passes

New CI run: https://github.com/matrixorigin/matrixone/actions/runs/34674424886

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

Labels

kind/bug Something isn't working size/L Denotes a PR that changes [500,999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: DECIMAL arithmetic does not widen results beyond Decimal128

4 participants