planner: deep copy AST-owned FieldTypes when building expressions#66652
planner: deep copy AST-owned FieldTypes when building expressions#66652ti-chi-bot[bot] merged 2 commits intopingcap:masterfrom
Conversation
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
|
Hi @AilinKid. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughDeep-copies AST-owned FieldType objects when building expression return types and planner-created constants; adds tests validating type isolation; documents and exposes some AST-backed metadata fields on logical and physical operator structs; adds a test dependency in BUILD.bazel. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.5.0)Command failed 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 |
|
/retest-required |
|
@AilinKid: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #66652 +/- ##
================================================
- Coverage 77.6714% 77.6271% -0.0444%
================================================
Files 2008 1929 -79
Lines 549230 537835 -11395
================================================
- Hits 426595 417506 -9089
+ Misses 120964 120316 -648
+ Partials 1671 13 -1658
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: guo-shaoge, qw4990 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
What problem does this PR solve?
Issue Number: close #66651
Problem Summary:
This PR implements step 1 of issue #66651. It also records the step 2 examination result for the remaining planner-held AST references.
When the planner rebuilds logical plans directly from a shared AST, some planner paths still reuse AST-owned mutable
FieldTypeobjects. That can make different builds share return-type state through expressions or planner-created constants.What changed and how does it work?
FieldTypepointers inexpression_rewriter.gowithDeepCopy()before building scalar functions or cast/json helper expressions.driver.ValueExpr.Typein planner-created constants withDeepCopy()inplanbuilder.go.RetTypestate, and updateBUILD.bazelfor the new test dependency.AstIndexHints,TableAsName,PartitionNames,SelectLockInfo, and SHOW-specific column/limit metadata.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Tests
GOCACHE=/tmp/tidb-go-build-cache go test -run 'TestCastRetTypeDoesNotShareASTFieldType|TestGetInsertColExprDeepCopiesValueExprFieldType' -tags=intest,deadlock ./pkg/planner/coremake bazel_lint_changedSummary by CodeRabbit
Bug Fixes
Tests
Documentation
Enhancements