fix(plan): align coalesce decimal scale across branches - #24842
Conversation
issue matrixorigin#24565 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). The companion decimal256-comparison part of issue matrixorigin#24565 is already handled on main; this PR only adds the missing COALESCE alignment. Added coverage: coalesce scale-alignment / already-aligned unit tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
issue matrixorigin#24565 BVT reproduction for the COALESCE decimal scale alignment fix, plus regression coverage for comparing a decimal256 CASE result with a decimal128 value. Result rows are appended in the existing format. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
matrix-meow
added
size/L
… promotion" This reverts commit c8a791f.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
I did not find a new hard correctness blocker in the implementation itself, but with a high bar I still think the regression coverage is too narrow for a user-visible type-inference fix.
Right now the added tests do prove the core bug is fixed for constant-expression cases, which is good. What is still missing are the more realistic/product-level cases where these decimal-type regressions usually hide:
-
column-based COALESCE
The new BVT coverage is still mostly constant-expression driven. I would want at least one table-backed case likeCOALESCE(dec23_2_col, dec38_7_col)with rows where the first branch is NULL and non-NULL, so we lock the real execution path rather than only planner/unit helpers. -
mixed integer + decimal branches
This PR touches numeric branch unification, but I don’t see a meaningful BVT for cases likeCOALESCE(int_col, decimal_col)orCOALESCE(NULL, 1, CAST(... AS DECIMAL(...))). That is a very common regression surface. -
3+ branch COALESCE
The current coverage is still mostly 2-branch oriented. A case likeCOALESCE(NULL, dec_a, dec_b)orCOALESCE(NULL, NULL, dec_expr)would better prove the final chosen decimal metadata is stable when more than two branches participate. -
metadata/type visibility
Since the bug is fundamentally about inferred decimal width/scale, I would also like one SQL-level metadata check (for example throughCREATE VIEW+DESC) so we know the visible column type is correct, not just the runtime value.
So my conclusion is: the main bug is covered, but the user-visible regression surface is still thinner than I would like. I would be much more comfortable after a small set of realistic SQL/BVT cases is added.
issue matrixorigin#24565 Address review feedback on matrixorigin#24842: extend COALESCE decimal coverage beyond constant-expression cases to product-level scenarios. - column-based COALESCE over decimal branches with null/non-null rows (COALESCE(dec(23,2), dec(38,7)) -> DECIMAL(38,7)) - mixed integer + decimal branches (column COALESCE(int, dec(20,5)) and constant COALESCE(NULL, 1, CAST(.. AS DECIMAL))) - 3+ branch COALESCE (NULL + two decimals, and all-NULL leading branches) - visible inferred decimal type via CREATE VIEW + DESC (Type = DECIMAL(38,7)) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
The fix in func_binary.go is correct, but the BVT coverage is too thin — constants only. The 4.0-dev backport (#24899) already has the full BVT suite this needs: column-based COALESCE over mixed-scale decimals, mixed integer+decimal branches, 3-branch decimal cases, and a CREATE VIEW + DESC metadata check. Please port that BVT coverage here.
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
The BVT coverage is actually complete — column-based, mixed int+decimal, 3-branch, and VIEW+DESC metadata check are all present. The fix in coalesceCheck is correct. No concerns.
Merge Queue Status
This pull request spent 1 hour 4 minutes 55 seconds in the queue, including 1 hour 4 minutes 23 seconds running CI. Required conditions to merge
|
5 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #24565
What this PR does / why we need it:
COALESCEover decimal branches with different scales returned the firstbranch's scale while carrying another branch's raw value, magnifying the
result (e.g. by 10^5):
coalesceCheckshort-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
decimalInputsAlignedguard).The companion decimal256-comparison scenario of issue #24565 (a decimal256
CASEresult compared with a decimal128) is already handled onmain, so thisPR only adds the missing
COALESCEalignment. The3.0-devbranch lacks thedecimal256 comparison support, so that part is delivered separately in #24841.
Added coverage:
Test_CoalesceCheck_DecimalScaleAlignment/Test_CoalesceCheck_DecimalAligned_NoCastValidation:
go test ./pkg/sql/plan/function,go vet,gofmtmainbuild: the COALESCE result is no longer magnifiedcase_when.sql(incl. the COALESCE repro) passed 101/101 locally🤖 Generated with Claude Code