planner, expression: reuse ODKU expressions#69936
Conversation
ti-chi-bot
Bot
added
the
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
size/XXL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a default-enabled ChangesON DUPLICATE expression reuse
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SessionVars
participant PlanBuilder
participant ExpressionRewriter
participant InsertResolveIndices
SessionVars->>PlanBuilder: Enable ON DUPLICATE expression reuse
PlanBuilder->>ExpressionRewriter: Rewrite assignments with memo
ExpressionRewriter->>ExpressionRewriter: Reuse eligible rewritten subexpressions
PlanBuilder->>InsertResolveIndices: Resolve rewritten expressions
InsertResolveIndices->>InsertResolveIndices: Reuse resolved expression trees
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/expression/builtin_clone_with_args.go`:
- Around line 637-641: Update builtinIntervalIntSig.CloneWithArgs to copy the
source instance’s hasNullable value onto newSig after cloning the base function,
matching builtinIntervalRealSig.CloneWithArgs so cloned INTERVAL expressions
preserve nullable evaluation behavior.
In `@pkg/sessionctx/variable/tidb_vars.go`:
- Line 1441: Add a Go doc comment immediately before
DefTiDBEnableOnDuplicateExpressionReuse that begins with the constant name and
documents its purpose as the default setting for enabling ON DUPLICATE
expression reuse.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a5d9b9ca-d353-4add-872b-16b099af1afe
📒 Files selected for processing (19)
docs/note/insert_on_duplicate_update_expression_reuse_summary.mdpkg/expression/BUILD.bazelpkg/expression/builtin.gopkg/expression/builtin_clone_with_args.gopkg/expression/builtin_regexp.gopkg/expression/builtin_vectorized_test.gopkg/expression/scalar_function.gopkg/expression/scalar_function_test.gopkg/planner/core/common_plans.gopkg/planner/core/expression_rewriter.gopkg/planner/core/plan_cache_test.gopkg/planner/core/plan_cache_utils.gopkg/planner/core/planbuilder.gopkg/planner/core/planbuilder_test.gopkg/planner/core/resolve_indices.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/setvar_affect.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/tidb_vars.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/release-8.5-materialized-view #69936 +/- ##
=============================================================================
- Coverage 57.7421% 57.1888% -0.5533%
=============================================================================
Files 1815 1817 +2
Lines 659304 668823 +9519
=============================================================================
+ Hits 380696 382492 +1796
- Misses 252162 259783 +7621
- Partials 26446 26548 +102
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/test unit-test |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/planbuilder.go (1)
4870-4872: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAbort the AST traversal once the threshold is reached.
While
Entercorrectly stops descending into child nodes by returningtrueforskipChildren, returningtruefromLeaveinstructs the visitor to continue processing siblings and the rest of the AST horizontally.You can completely short-circuit and terminate the entire AST walk immediately once the threshold is met by returning
falsefromLeave.💡 Proposed change to abort the visitor
-func (*onDuplicateFunctionCounter) Leave(in ast.Node) (ast.Node, bool) { - return in, true +func (c *onDuplicateFunctionCounter) Leave(in ast.Node) (ast.Node, bool) { + return in, c.count < minOnDuplicateFunctionCountForExpressionReuse }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/planbuilder.go` around lines 4870 - 4872, Update onDuplicateFunctionCounter.Leave to return false instead of true, so AST traversal terminates immediately when the duplicate-function threshold has been reached. Leave Enter and the node return value unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/planner/core/planbuilder.go`:
- Around line 4870-4872: Update onDuplicateFunctionCounter.Leave to return false
instead of true, so AST traversal terminates immediately when the
duplicate-function threshold has been reached. Leave Enter and the node return
value unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4ad22610-025e-4ce3-be1a-cf7fbc0b6287
📒 Files selected for processing (22)
docs/note/insert_on_duplicate_update_expression_reuse_summary.mdpkg/expression/BUILD.bazelpkg/expression/builtin.gopkg/expression/builtin_clone_with_args.gopkg/expression/builtin_compare.gopkg/expression/builtin_compare_test.gopkg/expression/builtin_regexp.gopkg/expression/builtin_vectorized_test.gopkg/expression/scalar_function.gopkg/expression/scalar_function_test.gopkg/planner/core/common_plans.gopkg/planner/core/expression_rewriter.gopkg/planner/core/integration_test.gopkg/planner/core/plan_cache_test.gopkg/planner/core/plan_cache_utils.gopkg/planner/core/planbuilder.gopkg/planner/core/planbuilder_test.gopkg/planner/core/resolve_indices.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/setvar_affect.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/tidb_vars.go
🚧 Files skipped from review as they are similar to previous changes (15)
- pkg/sessionctx/variable/tidb_vars.go
- pkg/sessionctx/variable/session.go
- pkg/sessionctx/variable/setvar_affect.go
- pkg/planner/core/plan_cache_test.go
- pkg/expression/builtin_regexp.go
- pkg/expression/scalar_function.go
- pkg/sessionctx/variable/sysvar.go
- pkg/planner/core/common_plans.go
- pkg/expression/builtin_vectorized_test.go
- pkg/planner/core/plan_cache_utils.go
- pkg/expression/builtin.go
- pkg/planner/core/planbuilder_test.go
- pkg/planner/core/integration_test.go
- pkg/planner/core/expression_rewriter.go
- pkg/expression/builtin_clone_with_args.go
| type onDuplicateExprMemo struct { | ||
| cache map[string]expression.Expression | ||
| frames []onDuplicateMemoFrame | ||
| restoreBuf bytes.Buffer | ||
| restoreCtx *format.RestoreCtx | ||
|
|
||
| keyableNodes map[ast.Node]struct{} | ||
| depthFrames []onDuplicateExprDepthFrame | ||
| } | ||
|
|
||
| const maxOnDuplicateExprReuseKeyDepth = 16 | ||
|
|
||
| type onDuplicateMemoFrame struct { |
There was a problem hiding this comment.
Could you add some new comments for these newly added structures and their fields?
| func (er *expressionRewriter) Enter(inNode ast.Node) (ast.Node, bool) { | ||
| if er.onDuplicateMemo != nil { | ||
| frame := onDuplicateMemoFrame{stackLen: er.ctxStackLen()} | ||
| if _, ok := inNode.(*ast.SetCollationExpr); ok { |
There was a problem hiding this comment.
Why do we need to handle setcollation specially here? Please add some comments especially some examples..
| return err | ||
| } | ||
| if !p.isOnDuplicateExpressionReuseEnabled() { | ||
| for _, asgn := range p.OnDuplicate { |
There was a problem hiding this comment.
L874-905 and L909-940 seem duplicated, could we merge them into one branch? For example, if !p.isOnDuplicateExpressionReuseEnabled() just set these 2 memo to nil, and let resolveExprIndicesWithMemo hand these 2 scenarios seperately.
|
/test check-dev2 |
1 similar comment
|
/test check-dev2 |
2 participants
What problem does this PR solve?
Issue Number: ref #65003
Problem Summary:
INSERT ... ON DUPLICATE KEY UPDATEstatements with many repeated assignment expressions can spend excessive CPU time and memory allocation in the planner compile path. In large ODKU statements, common sub-expressions are repeatedly rewritten, resolved, and cloned even when they are semantically identical across assignments.What changed and how does it work?
This PR optimizes the planner compile path for repeated ODKU expressions:
tidb_enable_on_duplicate_expression_reuseto control the optimization, includingSET_VARsupport.ResolveIndiceswith a memoized resolve path.CloneWithArgssupport for scalar functions to avoid deep-cloning already shared child expressions during memoized resolve.docs/note.Check List
Tests
Manual test:
Side effects
Documentation
Release note
Summary by CodeRabbit
INSERT ... ON DUPLICATE KEY UPDATE：复用公共子表达式，降低编译开销；并提供复杂度门控避免不适用场景。tidb_enable_on_duplicate_expression_reuse（默认开启），支持会话/全局/语句提示；启用/禁用会影响计划缓存键。