Skip to content

fix(plan): preserve decimal scale casts in case and iff#24572

Merged
XuPeng-SH merged 16 commits into
matrixorigin:mainfrom
ck89119:issue-24565
May 28, 2026
Merged

fix(plan): preserve decimal scale casts in case and iff#24572
XuPeng-SH merged 16 commits into
matrixorigin:mainfrom
ck89119:issue-24565

Conversation

@ck89119
Copy link
Copy Markdown
Contributor

@ck89119 ck89119 commented May 25, 2026

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #24565

What this PR does / why we need it:

This PR fixes a CASE/IFF type-binding bug for mixed DECIMAL scales.

Before this change, CASE WHEN and IFF could treat DECIMAL branches with the same OID but different scale/width as already matched, so no implicit cast was inserted. In the reported case, the THEN branch stayed at DECIMAL(23,2) while the ELSE branch produced a multiplication result with scale 7, and the final CASE result was formatted with the wrong scale.

This change makes CASE/IFF require casts whenever branch types are not exactly equal to the computed common type, including DECIMAL scale/width differences. It also preserves the maximum DECIMAL scale and width from source branch types when deriving the common return type.

Added coverage:

  • focused unit tests for caseCheck and iffCheck
  • binder test covering mixed DECIMAL scales in CASE
  • BVT reproduction in test/distributed/cases/expression/case_when.sql

Validation:

  • mo-tester generate/update result for case_when.sql
  • mo-tester run for case_when.sql: 87/87 passed
  • go test ./pkg/sql/plan/function -count=1
  • go test ./pkg/sql/plan -count=1
  • git diff --check
  • make
  • make static-check

@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 →

@ck89119
Copy link
Copy Markdown
Contributor Author

ck89119 commented May 25, 2026

Addressed the review concerns in follow-up commit a22f3ab3b.

Changes:

  • Restored setMaxScaleFromSource to scale-only behavior.
  • Computed a safe common DECIMAL width/scale for CASE/IFF with max(width-scale) + max(scale).
  • Narrowed metadata-based implicit cast injection back to DECIMAL only, so same-OID non-DECIMAL types are not pulled into new cast paths.
  • Strengthened unit coverage with width assertions.
  • Added SQL-level regressions for CASE THEN cast path and IFF mixed DECIMAL scales.

Validation rerun locally:

go test ./pkg/sql/plan/function -run Test_(CaseCheck|IffCheck)_DifferentDecimalScale -count=1
go test ./pkg/sql/plan -run TestBindFuncExprImplByPlanExpr_CaseDifferentDecimalScale -count=1

Prior full checks on this patch set also passed: go test ./pkg/sql/plan/function -count=1, go test ./pkg/sql/plan -count=1, BVT case_when.sql, make, and make static-check.

Note: the temporary pkg/sql/colexec/table_clone/table_clone.go net diff mentioned in this follow-up was removed from the PR scope in commit 3fc5af2b0.

ck89119 and others added 9 commits May 26, 2026 15:07
…3.0-dev

- Add T_decimal256 dispatch in caseFn/iffFn runtime switches
- Add types.Decimal256 to generalCaseFn/generalIffFn type constraints
- Add T_decimal256 to supportedTypeCast for T_decimal128
- Implement decimal128ToDecimal256 cast using Decimal256FromDecimal128
- Add execution-level unit tests for caseFn/iffFn with decimal256 result
- Add BVT coverage for CASE/IFF with decimal128 branches that promote to decimal256

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
(cherry picked from commit 534c092)
Copy link
Copy Markdown
Contributor

@XuPeng-SH XuPeng-SH left a comment

Choose a reason for hiding this comment

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

Reviewed from multiple angles: the CASE/IFF decimal metadata fix is sound, the decimal width/scale promotion logic is consistent, targeted Go tests with -race passed, and the actual diff matches the intended scope. Approving.

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/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants