fix(plan): align coalesce decimal scale and support decimal256 comparison by ck89119 · Pull Request #24841 · matrixorigin/matrixone

ck89119 · 2026-06-04T08:12:58Z

What type of PR is this?

Which issue(s) this PR fixes:

issue #24565

What this PR does / why we need it:

PR #24572 / #24589 fixed the mixed-DECIMAL scale magnification for CASE/IFF
but missed two related cases:

COALESCE scale magnification. COALESCE over decimal branches with
different scales returned the first branch's scale while carrying another
branch's raw value, magnifying the result (e.g. by 10^5):
```
SELECT COALESCE(
  CAST(NULL AS DECIMAL(23,2)),
  7.01970 * CAST(-58140.00 AS DECIMAL(23,2))
);
```
coalesceCheck short-circuited on a direct match without aligning
scale/width. It now falls through to the alignment cast path unless every
decimal branch already shares the same scale and width (new
decimalInputsAligned guard).
decimal256 comparison. A CASE/IFF whose branches need more than 38
digits is promoted to decimal256, but comparing such a value with a
decimal128 failed with ERROR 20203: invalid argument operator =, bad value [DECIMAL256 DECIMAL128]:
```
SELECT (CASE WHEN 1=1 THEN CAST(1 AS DECIMAL(38,0))
             ELSE CAST(0 AS DECIMAL(38,20)) END)
     = CAST(1 AS DECIMAL(38,20));
```
decimal256 was unsupported across the whole comparison path.
fixedTypeCastRule1 now promotes a decimal-vs-decimal pair involving
decimal256 to decimal256 (neutral for arithmetic operators, whose support
checks still reject decimal256); the equal/other comparison support
functions accept decimal256; and a new valueDec256Compare plus decimal256
branches in the six comparison executors and nullSafeEqualFn evaluate it.

Scope is limited to decimal-vs-decimal comparisons.

Added coverage:

unit tests for fixedTypeCastRule1 decimal256 promotion, comparison support,
and the decimal256 comparison executors
coalesce scale-alignment / already-aligned unit tests
BVT reproduction in test/distributed/cases/expression/case_when.sql

Validation:

go test ./pkg/sql/plan/function, go test ./pkg/sql/plan
go vet, gofmt, molint, golangci-lint (0 issues) on the changed package
mo-tester: case_when.sql 100%, func_coalesce_1.sql 100%, decimal256_insert.sql 100%

🤖 Generated with Claude Code

…ison issue matrixorigin#24565 PR matrixorigin#24572 fixed the mixed-DECIMAL scale magnification for CASE/IFF but missed two related cases: 1. COALESCE over decimal branches with different scales returned the first branch's scale while carrying another branch's raw value, magnifying the result (e.g. 10^5). coalesceCheck short-circuited on a direct match without aligning scale/width. It now falls through to the alignment cast path unless all decimal branches already share the same scale and width (new decimalInputsAligned guard). 2. A CASE/IFF whose branches need more than 38 digits is promoted to decimal256, but comparing such a value with a decimal128 failed with "bad value [DECIMAL256 DECIMAL128]". Decimal256 was unsupported across the whole comparison path. fixedTypeCastRule1 now promotes a decimal-vs-decimal pair involving decimal256 to decimal256 (neutral for arithmetic, whose support checks still reject decimal256); the equal/other comparison support functions accept decimal256; and a new valueDec256Compare plus decimal256 branches in the six comparison executors and nullSafeEqualFn evaluate it. Scope limited to decimal-vs-decimal comparisons. Added coverage: - unit tests for fixedTypeCastRule1 decimal256 promotion, comparison support, and the decimal256 comparison executors - coalesce scale-alignment / already-aligned unit tests - BVT reproduction in test/distributed/cases/expression/case_when.sql Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

qodo-code-review · 2026-06-04T08:13:04Z

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 →

issue matrixorigin#24565 Follow-up to review on matrixorigin#24841: 1. coalesceCheck now derives the common decimal type from max(integral width) + max(scale), promoting to decimal256 when it overflows decimal128, instead of max(width)/max(scale). This keeps integer capacity for branches like COALESCE(DECIMAL(38,0), DECIMAL(30,30)) (now DECIMAL256(68,30)) rather than truncating to DECIMAL128. Adds a decimal256 COALESCE overload and Decimal256 to NormalType. 2. valueDec256Compare now propagates the error from Decimal256.Scale() instead of comparing un-scaled raw values, so an extreme scale-span overflow reports an error rather than returning a wrong boolean. (The cross-family alignment overflow at the cast stage is a known decimal256 limitation, shared with the existing decimal compare path.) 3. The case_when BVT result is regenerated as an append-only diff, without the unrelated whole-file format rewrite. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Address review findings on decimal256 support (issue matrixorigin#24565): - between: compute one common decimal type preserving the max integral width AND max scale across the three operands, promoting to decimal256 when decimal128 would overflow, instead of only aligning scale (which could keep an under-sized width/family and overflow) or rejecting mixed-family decimals. - comparison: extend the decimal256 cast shortcut to decimal256-vs-integer so bare integer literals bind, and add T_decimal256 to isNumericType for the runtime type-mismatch fallback path. - convert case_when.sql to LF line endings. Add UTs (decimal helpers, integer promotion, between promotion) and BVT cases covering decimal256-vs-integer and the between decimal256 promotion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…ue-24565-decimal256

aunjgr

Two fixes:

1. COALESCE decimal scale alignment (func_binary.go). When multiple decimal branches have different scales (e.g., COALESCE(NULL, decimal(10,2), decimal(10,4))), previously the result inherited the first matching branch's scale. This magnified values (issue #24565). Fix: coalesceDecimalResult computes the max integral width+scale across all inputs, promoting to decimal256 when combined precision overflows decimal128. Also fixes matchDirectly short-circuit for decimals — previously returned immediately with the first match's scale; now continues to find the best aligned type.

2. Decimal256 comparison operators (func_compare.go +255). Six comparison functions (=, <>, <, <=, >, >=) now support T_decimal256 via types.CompareDecimal256. The existing generic path used compareGeneric which doesn't handle decimal256 correctly (treats as raw bytes). Tests in func_compare_decimal256_test.go cover all six operators + boundary values + NULL handling.

Additional: operator_between.go (+4) — adds T_decimal256 to the BETWEEN/in_range guard that skips fixed-type ops for unsupported types. type_check.go (+13) — adds decimal256 to comparison type check.

LGTM.

XuPeng-SH

LGTM after rechecking from three angles:

correctness: the earlier decimal256 BETWEEN gap is fixed in both planner and runtime paths
regression risk: decimal256 support remains scoped to decimal-vs-decimal comparisons / aligned casts
tests: coverage now includes the non-constant decimal256 BETWEEN runtime path in addition to planner checks and SQL repros

mergify · 2026-06-05T16:55:49Z

ck89119 requested review from aunjgr, heni02 and ouyuanning as code owners June 4, 2026 08:12

ck89119 had a problem deploying to ci June 4, 2026 08:13 — with GitHub Actions Failure

ck89119 temporarily deployed to ci June 4, 2026 08:13 — with GitHub Actions Inactive

ck89119 had a problem deploying to ci June 4, 2026 08:13 — with GitHub Actions Failure

ck89119 temporarily deployed to ci June 4, 2026 08:13 — with GitHub Actions Inactive

ck89119 had a problem deploying to ci June 4, 2026 08:13 — with GitHub Actions Failure

ck89119 temporarily deployed to ci June 4, 2026 08:13 — with GitHub Actions Inactive

ck89119 had a problem deploying to ci June 4, 2026 08:13 — with GitHub Actions Failure

matrix-meow added the size/L Denotes a PR that changes [500,999] lines label Jun 4, 2026

mergify Bot requested a review from XuPeng-SH June 4, 2026 08:13

mergify Bot added the kind/bug Something isn't working label Jun 4, 2026

ck89119 mentioned this pull request Jun 4, 2026

fix(plan): align coalesce decimal scale across branches #24842

Draft

7 tasks

ck89119 temporarily deployed to ci June 4, 2026 13:39 — with GitHub Actions Inactive

ck89119 had a problem deploying to ci June 4, 2026 13:39 — with GitHub Actions Failure

ck89119 temporarily deployed to ci June 5, 2026 02:41 — with GitHub Actions Inactive

ck89119 had a problem deploying to ci June 5, 2026 02:41 — with GitHub Actions Failure

ck89119 temporarily deployed to ci June 5, 2026 02:41 — with GitHub Actions Inactive

ck89119 had a problem deploying to ci June 5, 2026 03:34 — with GitHub Actions Failure

ck89119 temporarily deployed to ci June 5, 2026 03:34 — with GitHub Actions Inactive

ck89119 had a problem deploying to ci June 5, 2026 07:13 — with GitHub Actions Failure

ck89119 temporarily deployed to ci June 5, 2026 08:12 — with GitHub Actions Inactive

ck89119 had a problem deploying to ci June 5, 2026 08:12 — with GitHub Actions Failure

ck89119 temporarily deployed to ci June 5, 2026 08:12 — with GitHub Actions Inactive

ck89119 had a problem deploying to ci June 5, 2026 08:12 — with GitHub Actions Error

ck89119 temporarily deployed to ci June 5, 2026 08:12 — with GitHub Actions Inactive

ck89119 had a problem deploying to ci June 5, 2026 08:12 — with GitHub Actions Error

ck89119 temporarily deployed to ci June 5, 2026 08:12 — with GitHub Actions Inactive

matrix-meow added size/XL Denotes a PR that changes [1000, 1999] lines and removed size/L Denotes a PR that changes [500,999] lines labels Jun 5, 2026

Merge branch '3.0-dev' into issue-24565-decimal256

9a75b39

ck89119 had a problem deploying to ci June 5, 2026 09:04 — with GitHub Actions Error

ck89119 temporarily deployed to ci June 5, 2026 09:04 — with GitHub Actions Inactive

ck89119 added 2 commits June 5, 2026 17:29

fix

d22f73c

Merge remote-tracking branch 'origin/issue-24565-decimal256' into iss…

d22a681

…ue-24565-decimal256

aunjgr approved these changes Jun 5, 2026

View reviewed changes

XuPeng-SH approved these changes Jun 5, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(plan): align coalesce decimal scale and support decimal256 comparison#24841

fix(plan): align coalesce decimal scale and support decimal256 comparison#24841
mergify[bot] merged 8 commits into
matrixorigin:3.0-devfrom
ck89119:issue-24565-decimal256

ck89119 commented Jun 4, 2026

Uh oh!

qodo-code-review Bot commented Jun 4, 2026

Uh oh!

aunjgr left a comment

Uh oh!

XuPeng-SH left a comment

Uh oh!

mergify Bot commented Jun 5, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

ck89119 commented Jun 4, 2026

What type of PR is this?

Which issue(s) this PR fixes:

What this PR does / why we need it:

Uh oh!

qodo-code-review Bot commented Jun 4, 2026

Qodo reviews are paused for this user.

Uh oh!

aunjgr left a comment

Choose a reason for hiding this comment

Uh oh!

XuPeng-SH left a comment

Choose a reason for hiding this comment

Uh oh!

mergify Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mergify Bot commented Jun 5, 2026 •

edited

Loading