planner: Fix expression rewriting and method signature mismatch in plan cache #58506

dash12653 · 2024-12-24T09:15:13Z

What problem does this PR solve?

Issue Number: close #56772

Problem Summary:

What changed and how does it work?

In the expression rewriting process, when constructing the condition t1.col1 + 10 > ? + 10, the argument ? + 10 will go through argument refinement, and the final result is set to NULL. The type of NULL is determined as int.
Then, the method signature for t1.col1 + 10 > ? + 10 is set to builtinArithmeticPlusIntSig. But the DeferredExpr for the expression plus(NULL, 10) is not removed, which results in a conflict method signature of builtinArithmeticPlusRealSig,
which cannot handle int types. This leads to an error during execution.

RemoveMutableConst(): will not traverse all the expressions.

tidb/pkg/expression/util.go

Lines 1583 to 1602 in 8ecdb54

    
           func RemoveMutableConst(ctx BuildContext, exprs []Expression) (err error) { 
        
           	for _, expr := range exprs { 
        
           		switch v := expr.(type) { 
        
           		case *Constant: 
        
           			v.ParamMarker = nil 
        
           			if v.DeferredExpr != nil { // evaluate and update v.Value to convert v to a complete immutable constant. 
        
           				// TODO: remove or hide DeferredExpr since it's too dangerous (hard to be consistent with v.Value all the time). 
        
           				v.Value, err = v.DeferredExpr.Eval(ctx.GetEvalCtx(), chunk.Row{}) 
        
           				if err != nil { 
        
           					return err 
        
           				} 
        
           				v.DeferredExpr = nil 
        
           			} 
        
           			v.DeferredExpr = nil // do nothing since v.Value has already been evaluated in this case. 
        
           		case *ScalarFunction: 
        
           			return RemoveMutableConst(ctx, v.GetArgs()) 
        
           		} 
        
           	} 
        
           	return nil 
        
           }

I modified the code to ensure that all expressions are properly traversed and DeferredExpr would be removed.

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.

Side effects

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

Documentation

Release note

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

None

ti-chi-bot · 2024-12-24T09:15:23Z

Hi @dash12653. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

tiprow · 2024-12-24T09:15:28Z

Hi @dash12653. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

dash12653 · 2025-01-07T00:59:55Z

@qw4990 @AilinKid PTAL

dash12653 · 2025-03-03T08:46:18Z

@qw4990 @AilinKid Hi, could you help to review this pr when you have a chance? Thanks!

AilinKid · 2025-07-07T07:14:25Z

pkg/expression/util.go

@@ -1599,7 +1599,10 @@ func RemoveMutableConst(ctx BuildContext, exprs []Expression) (err error) {
 			}
 			v.DeferredExpr = nil // do nothing since v.Value has already been evaluated in this case.
 		case *ScalarFunction:
-			return RemoveMutableConst(ctx, v.GetArgs())
+			err := RemoveMutableConst(ctx, v.GetArgs())


AilinKid · 2025-07-07T07:15:59Z

sorry for didn't notice github notification for sometime, do you mind resolve conflict it runs into

ti-chi-bot · 2025-07-07T07:16:38Z

[LGTM Timeline notifier]

Timeline:

2025-07-07 07:14:37.820123651 +0000 UTC m=+1897530.543302628: ☑️ agreed by AilinKid.
2025-07-07 07:16:37.176735359 +0000 UTC m=+1897649.899914340: ☑️ agreed by qw4990.

dash12653 · 2025-07-07T08:08:52Z

PTAL @AilinKid @qw4990

windtalker

lgtm

ti-chi-bot · 2025-07-08T04:06:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AilinKid, qw4990, windtalker

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [AilinKid,qw4990,windtalker]
~~pkg/expression/OWNERS~~ [windtalker]

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

windtalker · 2025-07-08T04:14:19Z

/ok-to-test

codecov · 2025-07-08T04:35:05Z

Codecov Report

Attention: Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 73.3606%. Comparing base (ee9f230) to head (436f959).
Report is 11 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #58506        +/-   ##
================================================
+ Coverage   72.9617%   73.3606%   +0.3988%     
================================================
  Files          1750       1751         +1     
  Lines        484506     489486      +4980     
================================================
+ Hits         353504     359090      +5586     
+ Misses       109381     108536       -845     
- Partials      21621      21860       +239

Flag	Coverage Δ
integration	`42.3696% <25.0000%> (?)`
unit	`72.1897% <25.0000%> (-0.0287%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.7804% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`46.2387% <ø> (-0.0911%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ti-chi-bot · 2025-07-08T05:02:12Z

In response to a cherrypick label: new pull request created to branch release-8.5: #62272.
But this PR has conflicts, please resolve them!

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2025-07-08T05:02:54Z

In response to a cherrypick label: new pull request created to branch release-7.5: #62273.
But this PR has conflicts, please resolve them!

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2025-07-08T05:03:36Z

In response to a cherrypick label: new pull request created to branch release-8.1: #62274.
But this PR has conflicts, please resolve them!

…fix-scalar-func-init * commit '5ec4f36c855574b64be98e4c9f6b4485ceb4143d': (48 commits) dxf: use correct store in add-index, checksum by copr in import-into, and enable DXF service (pingcap#62224) infoschema: Add new fields in tiflash system table (pingcap#62296) lightning: fix panic on checksum manager close when checksum is off (pingcap#62300) *: Add owners cfg file for pkg/metrics module (pingcap#62292) autoid: reserve a table ID range for downstream fork (pingcap#62157) planner: Fix expression rewriting and method signature mismatch in plan cache (pingcap#58506) planner: use prop based noCopPushDown mechanism to replace aggregation field. (pingcap#62249) fix: close issue 59457 by trim compare first (pingcap#61915) planner: add skew risk ratio for range pred (pingcap#62035) meta: unify definition of system or memory DB name (pingcap#62247) ddl: add retry for updateSelfVersion (pingcap#62190) *: upgrade client go to add config valid function (pingcap#62246) executor: fix the inappropriate RequiredRows set by `TopNExec` (pingcap#62154) planner: RegardNULLAsPoint should be true as default (pingcap#62194) planner: apply predicate simplification before extract condition (pingcap#62211) test: fix failed test caused by new version of `mc` (pingcap#61356) planner: deprecate the logical interface CanPushToCop and its implementation canPushToCopImpl. (pingcap#62235) fix: shut down test without error (pingcap#61921) planner: lift the canPushToCop check of logical join/window/selection (pingcap#62206) backend/local: add rate limiter for split region and ingest data (pingcap#61555) ...

…an cache (#58506) (#62272) close #56772

…an cache (#58506) (#62273) close #56772

dash12653 closed this Mar 3, 2025

dash12653 force-pushed the tidb-56772 branch from af76047 to 448649e Compare March 3, 2025 08:11

ti-chi-bot bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 3, 2025

dash12653 reopened this Mar 3, 2025

ti-chi-bot bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 3, 2025

AilinKid approved these changes Jul 7, 2025

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jul 7, 2025

qw4990 approved these changes Jul 7, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 7, 2025

resolve conflict

436f959

dash12653 force-pushed the tidb-56772 branch from 3af0e23 to 436f959 Compare July 7, 2025 08:07

fixdb requested a review from windtalker July 8, 2025 03:54

windtalker approved these changes Jul 8, 2025

View reviewed changes

ti-chi-bot bot added approved and removed do-not-merge/needs-triage-completed labels Jul 8, 2025

ti-chi-bot bot merged commit 9accc3c into pingcap:master Jul 8, 2025
27 checks passed

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Jul 8, 2025

This is an automated cherry-pick of pingcap#58506

07f488e

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Jul 8, 2025

planner: Fix expression rewriting and method signature mismatch in plan cache (#58506) #62272

Merged

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Jul 8, 2025

This is an automated cherry-pick of pingcap#58506

49b7060

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Jul 8, 2025

planner: Fix expression rewriting and method signature mismatch in plan cache (#58506) #62273

Merged

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Jul 8, 2025

This is an automated cherry-pick of pingcap#58506

28957b0

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Jul 8, 2025

planner: Fix expression rewriting and method signature mismatch in plan cache (#58506) #62274

Open

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Jul 10, 2025

planner: Fix expression rewriting and method signature mismatch in pl…

7b22116

…an cache (#58506) (#62272) close #56772

ti-chi-bot bot pushed a commit that referenced this pull request Jul 15, 2025

planner: Fix expression rewriting and method signature mismatch in pl…

7b6c583

…an cache (#58506) (#62273) close #56772

	func RemoveMutableConst(ctx BuildContext, exprs []Expression) (err error) {
	for _, expr := range exprs {
	switch v := expr.(type) {
	case *Constant:
	v.ParamMarker = nil
	if v.DeferredExpr != nil { // evaluate and update v.Value to convert v to a complete immutable constant.
	// TODO: remove or hide DeferredExpr since it's too dangerous (hard to be consistent with v.Value all the time).
	v.Value, err = v.DeferredExpr.Eval(ctx.GetEvalCtx(), chunk.Row{})
	if err != nil {
	return err
	}
	v.DeferredExpr = nil
	}
	v.DeferredExpr = nil // do nothing since v.Value has already been evaluated in this case.
	case *ScalarFunction:
	return RemoveMutableConst(ctx, v.GetArgs())
	}
	}
	return nil
	}

planner: Fix expression rewriting and method signature mismatch in plan cache #58506

planner: Fix expression rewriting and method signature mismatch in plan cache #58506

Uh oh!

Conversation

dash12653 commented Dec 24, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

ti-chi-bot bot commented Dec 24, 2024

Uh oh!

tiprow bot commented Dec 24, 2024

Uh oh!

dash12653 commented Jan 7, 2025

Uh oh!

dash12653 commented Mar 3, 2025

Uh oh!

AilinKid Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

AilinKid commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ti-chi-bot bot commented Jul 7, 2025

[LGTM Timeline notifier]

Uh oh!

dash12653 commented Jul 7, 2025

Uh oh!

windtalker left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Jul 8, 2025

Uh oh!

windtalker commented Jul 8, 2025

Uh oh!

codecov bot commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

ti-chi-bot commented Jul 8, 2025

Uh oh!

ti-chi-bot commented Jul 8, 2025

Uh oh!

ti-chi-bot commented Jul 8, 2025

Uh oh!

Uh oh!

AilinKid commented Jul 7, 2025 •

edited

Loading

codecov bot commented Jul 8, 2025 •

edited

Loading