Skip to content

expression: clone constant before PropagateType mutation#68433

Merged
ti-chi-bot[bot] merged 3 commits into
pingcap:masterfrom
hawkingrei:issue-66706-decimal-scale-sign
May 19, 2026
Merged

expression: clone constant before PropagateType mutation#68433
ti-chi-bot[bot] merged 3 commits into
pingcap:masterfrom
hawkingrei:issue-66706-decimal-scale-sign

Conversation

@hawkingrei
Copy link
Copy Markdown
Member

@hawkingrei hawkingrei commented May 16, 2026

What problem does this PR solve?

Issue Number: close #66706

Problem Summary:

When a view projects a DECIMAL constant and an outer predicate applies SIGN() to that view column, the implicit cast-to-real path can propagate widened DECIMAL metadata back into the shared constant. The projected view column can then be displayed with a widened scale, for example 0.990000000000000000000000000000 instead of 0.99.

What changed and how does it work?

PropagateType now clones *expression.Constant before mutating propagated type metadata, matching the existing clone-before-mutate handling for Column and CorrelatedColumn. This keeps the cast-local widened metadata from leaking back into a shared view projection constant.

This PR also adds:

  • a focused pkg/expression unit assertion that the original constant keeps DECIMAL(5,2) while the cast-local clone gets the propagated type;
  • a planner issue regression for the direct view predicate and derived-table control query, run under both default and cascades planner modes;
  • test case map updates for the touched test files.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Manual test:

go test ./.tmp/issue66706 -run TestIssue66706Replay -count=1 -tags=intest,deadlock -v

Before the fix, the direct view-predicate query returned 0.990000000000000000000000000000 with result field DECIMAL(48,30), while the derived-table control returned 0.99. After the fix, both returned 0.99 and the direct result field stayed DECIMAL(5,2).

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Fix an issue that a DECIMAL constant projected from a view could be displayed with a widened scale when the view column was also used in a SIGN() predicate.

Summary by CodeRabbit

  • Bug Fixes

    • Constants are no longer mutated during numeric type propagation, preventing loss of scale/precision (fixes cases like SIGN() on view columns).
  • Tests

    • Added unit and regression tests to verify decimal constant handling, ensuring propagated expressions preserve numeric metadata and preventing regressions.

Review Change Stack

@ti-chi-bot ti-chi-bot Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/planner SIG: Planner labels May 16, 2026
@hawkingrei hawkingrei added the AI-Correction Bugfix by AI label May 16, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 16, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 783b865d-77dc-4e52-9ba3-eb1383a9219c

📥 Commits

Reviewing files that changed from the base of the PR and between f4d2d8b and f3ff867.

📒 Files selected for processing (3)
  • pkg/expression/expression.go
  • pkg/expression/expression_test.go
  • pkg/planner/core/issuetest/planner_issue_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • pkg/expression/expression_test.go
  • pkg/planner/core/issuetest/planner_issue_test.go

📝 Walkthrough

Walkthrough

Clone Constant nodes during ETReal type propagation to avoid mutating decimal metadata; add unit test and cascades regression test validating SIGN() on view-returned decimals preserves scale and emits no warnings.

Changes

Decimal scale leak fix in type propagation

Layer / File(s) Summary
Constant cloning in type propagation and unit test
pkg/expression/expression.go, pkg/expression/expression_test.go
PropagateType now clones *Constant arguments in the types.ETReal branch before adjusting type metadata. TestConstant adds coverage for decimal constants, asserting the propagated argument is a cloned instance with expected Flen/Decimal metadata while the original RetType remains unchanged.
Regression test for issue-66706
pkg/planner/core/issuetest/planner_issue_test.go
New cascades regression test issue-66706-decimal-scale-leak-through-sign-view-predicate verifies SELECT v0.c0 FROM v0 WHERE SIGN(v0.c0) returns 0.99 and produces no warnings, and repeats the check with a derived-table form.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • pingcap/tidb#66652: Related work handling deep-copying/ownership of FieldType/RetType for constants and planner-built expressions to avoid shared metadata mutation.

Suggested reviewers

  • winoros
  • qw4990
  • guo-shaoge

Poem

🐇 I cloned the constant, kept its scale bright,
No creeping zeros in the moonlit night.
SIGN() on views now sings true and small,
Precision preserved — rejoice, one and all! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'expression: clone constant before PropagateType mutation' clearly and specifically describes the main code change, matching the PR objectives and the core fix in pkg/expression/expression.go.
Description check ✅ Passed The description includes all required template sections: Issue Number (close #66706), Problem Summary, What Changed explanation, comprehensive Check List (unit test checked, manual test provided), Side effects assessment, Documentation (affects user behaviors and MySQL compatibility checked), and a well-formatted Release note.
Linked Issues check ✅ Passed The PR directly addresses all objectives from issue #66706 by cloning *expression.Constant before PropagateType mutation to prevent scale widening, adding unit tests validating constant preservation, planner regression tests covering both direct view predicates and derived-table equivalence, and provides a release note documenting the fix.
Out of Scope Changes check ✅ Passed All changes are directly scoped to issue #66706: the core fix in pkg/expression/expression.go clones constants during type propagation, the unit test verifies constant preservation during cast, and the planner regression test reproduces the original bug scenario. No unrelated changes detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.7084%. Comparing base (a62a6d1) to head (f3ff867).

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #68433        +/-   ##
================================================
- Coverage   77.2798%   75.7084%   -1.5715%     
================================================
  Files          2010       2008         -2     
  Lines        555326     563442      +8116     
================================================
- Hits         429155     426573      -2582     
- Misses       125251     136812     +11561     
+ Partials        920         57       -863     
Flag Coverage Δ
integration 41.5270% <ø> (+1.7340%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 60.4679% <ø> (ø)
parser ∅ <ø> (∅)
br 49.9534% <ø> (-13.0557%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hawkingrei
Copy link
Copy Markdown
Member Author

/retest-required

Copy link
Copy Markdown
Contributor

@windtalker windtalker left a comment

Choose a reason for hiding this comment

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

lgtm

@ti-chi-bot ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label May 19, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented May 19, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guo-shaoge, windtalker

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added approved lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 19, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented May 19, 2026

[LGTM Timeline notifier]

Timeline:

  • 2026-05-19 03:31:11.069116422 +0000 UTC m=+235000.573247097: ☑️ agreed by windtalker.
  • 2026-05-19 03:32:59.827437196 +0000 UTC m=+235109.331567872: ☑️ agreed by guo-shaoge.

@hawkingrei hawkingrei force-pushed the issue-66706-decimal-scale-sign branch from f4d2d8b to f3ff867 Compare May 19, 2026 04:02
@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

@ti-chi-bot ti-chi-bot Bot merged commit aaa5331 into pingcap:master May 19, 2026
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-Correction Bugfix by AI approved lgtm release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/planner SIG: Planner size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unexpected DECIMAL scale (30 zeros) in projection when evaluating SIGN() in WHERE clause on a View

3 participants