planner, sessionctx: remove index join build v1#66871
planner, sessionctx: remove index join build v1#66871AilinKid wants to merge 3 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
the
release-note
|
❌ Failed to Create Project We encountered an error while creating your project. Please try again or contact support if the issue persists. |
ti-chi-bot
bot
added
sig/planner
|
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. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. 📝 WalkthroughWalkthroughThis PR consolidates the index join build optimizer by removing the V1/V2 feature flag logic and simplifying plan construction. The V2 approach becomes the default behavior, with conditional enumeration paths unified and index-join runtime details deferred to a single completion function, resulting in substantial code reduction. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
[LGTM Timeline notifier]Timeline:
|
|
/ok-to-test |
|
/ok-to-test |
| s.AnalyzeVersion = tidbOptPositiveInt32(val, vardef.DefTiDBAnalyzeVersion) | ||
| return nil | ||
| }}, | ||
| {Scope: vardef.ScopeGlobal | vardef.ScopeSession, Name: vardef.TiDBOptIndexJoinBuild, Value: BoolToOnOff(vardef.DefTiDBOptIndexJoinBuild), Type: vardef.TypeBool, SetSession: func(s *SessionVars, val string) error { |
There was a problem hiding this comment.
Does this mean that when users try to set it, they will see a message indicating it doesn't exist? Typically, we don't directly remove the variable I guess, we could report an error stating that it is always on?
There was a problem hiding this comment.
This also means that once a user upgrades from the default disabled version, they will observe a change in behavior. I'm not sure if this is an issue.
There was a problem hiding this comment.
how does analyze v1 did, it the old variable is still there and settable?
There was a problem hiding this comment.
This also means that once a user upgrades from the default disabled version, they will observe a change in behavior. I'm not sure if this is an issue.
This could be included in our release notes document to notify the user, I suppose.
There was a problem hiding this comment.
make sense, addressed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/find_best_task.go (1)
692-694: Move this rationale next to theenumerationContainIndexJoin(...)guard.The note explains why index-join candidates must keep the enforced branch alive, but it currently sits inside the branch that only runs after both enumerations are known not to contain index joins. Rewording or moving it closer to Lines 681-688 would make the condition much easier to follow. As per coding guidelines, "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs, and SHOULD NOT restate what the code already makes clear".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/find_best_task.go` around lines 692 - 694, Move and reword the current rationale comment so it sits immediately adjacent to the enumerationContainIndexJoin(...) guard (near the code that checks whether either enumeration contains an index-join) instead of inside the later branch where both enumerations are known not to contain index joins; update the text to explain concisely that when an enumeration may contain an index-join the enforced branch must be preserved because emptying the sort item can enable additional index-join candidates or make hints applicable only after the inner plan is built, and remove any redundancy with the surrounding code so the comment documents the intent/constraint (enumerationContainIndexJoin, enforced branch, sort item, index-join candidates) rather than restating obvious implementation details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/planner/core/find_best_task.go`:
- Around line 692-694: Move and reword the current rationale comment so it sits
immediately adjacent to the enumerationContainIndexJoin(...) guard (near the
code that checks whether either enumeration contains an index-join) instead of
inside the later branch where both enumerations are known not to contain index
joins; update the text to explain concisely that when an enumeration may contain
an index-join the enforced branch must be preserved because emptying the sort
item can enable additional index-join candidates or make hints applicable only
after the inner plan is built, and remove any redundancy with the surrounding
code so the comment documents the intent/constraint
(enumerationContainIndexJoin, enforced branch, sort item, index-join candidates)
rather than restating obvious implementation details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a13c294a-58d7-4a71-adc6-ad696537123e
📒 Files selected for processing (8)
pkg/planner/core/casetest/join/join_test.gopkg/planner/core/exhaust_physical_plans.gopkg/planner/core/find_best_task.gopkg/planner/core/index_join_path.gopkg/planner/core/task.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/sysvar.go
💤 Files with no reviewable changes (5)
- pkg/planner/core/casetest/join/join_test.go
- pkg/sessionctx/variable/session.go
- pkg/sessionctx/vardef/tidb_vars.go
- pkg/sessionctx/variable/sysvar.go
- pkg/planner/core/index_join_path.go
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hawkingrei 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 |
|
/retest |
|
@AilinKid: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/retest |
3 participants
What problem does this PR solve?
Issue Number: ref #65781
Problem Summary:
Index join build v1 and the
tidb_opt_index_join_build_v2switch are still kept in the planner and session variable layer even though the refactored index join build path is already the only path we want to keep. The extra path adds dead code, extra maintenance cost, and one unnecessary user-visible variable.What changed and how does it work?
tidb_opt_index_join_build_v2sysvar and the matchingSessionVarsgateCheck List
Tests
Test commands:
make failpoint-enableenv GOCACHE=/tmp/codex-go-build GOTMPDIR=/tmp/codex-go-tmp go test -run TestJoinRegression -tags=intest,deadlock ./pkg/planner/coreenv GOCACHE=/tmp/codex-go-build GOTMPDIR=/tmp/codex-go-tmp go test -run 'TestDefaultValuesAreSettable|TestSysVarNameIsLowerCase|TestSettersandGetters' -tags=intest,deadlock ./pkg/sessionctx/variable/testsenv GOCACHE=/tmp/codex-go-build GOTMPDIR=/tmp/codex-go-tmp go test -run TestJoinRegression -tags=intest,deadlock ./pkg/planner/core/casetest/joinmake failpoint-disableenv GOCACHE=/tmp/codex-go-build GOTMPDIR=/tmp/codex-go-tmp GOLANGCI_LINT_CACHE=/tmp/codex-golangci-lint-cache make lintSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit