parser, sessionctx: add gated FULL OUTER JOIN syntax#69999
Conversation
ti-chi-bot
Bot
added
release-note
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ChangesFULL OUTER JOIN foundation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ScannerLex
participant ParserGrammar
participant BuildJoin
Client->>ScannerLex: submit FULL OUTER JOIN query
ScannerLex->>ParserGrammar: return fullJoinType
ParserGrammar->>BuildJoin: pass ast.FullJoin
BuildJoin->>Client: return FULL OUTER JOIN not supported
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
pkg/parser/ast/dml.go (1)
73-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the
FullJoinsemantic contract.
FullJoinspecifically representsFULL OUTER JOINin the AST, not theFULLidentifier/alias behavior. Please make the exported comment state that explicitly.Proposed wording
- // FullJoin is full join type. + // FullJoin represents a FULL OUTER JOIN.As per coding guidelines, exported-symbol doc comments should prefer semantic constraints over name restatement.
🤖 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/parser/ast/dml.go` around lines 73 - 74, Update the exported comment for the FullJoin constant to explicitly state that it represents the FULL OUTER JOIN AST type, distinguishing it from FULL identifier or alias behavior.Source: Coding guidelines
🤖 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/planner/core/logical_plan_builder.go`:
- Around line 828-830: Move the FULL OUTER JOIN rejection in the join-building
flow immediately after the joinNode.Right == nil guard and before lateral
detection or child-plan construction. Keep returning
plannererrors.ErrNotSupportedYet.GenWithStackByArgs("FULL OUTER JOIN") so every
parsed FULL OUTER JOIN consistently fails before child-planning errors or
lateral handling.
---
Nitpick comments:
In `@pkg/parser/ast/dml.go`:
- Around line 73-74: Update the exported comment for the FullJoin constant to
explicitly state that it represents the FULL OUTER JOIN AST type, distinguishing
it from FULL identifier or alias behavior.
🪄 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 Plus
Run ID: 6d19f0e7-44f5-4a34-aa66-2baa24c78c23
📒 Files selected for processing (13)
docs/note/fullouter_join_dev_note.mdpkg/parser/ast/dml.gopkg/parser/lexer.gopkg/parser/parser.gopkg/parser/parser.ypkg/parser/parser_test.gopkg/planner/core/logical_plan_builder.gopkg/planner/core/physical_plan_test.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/setvar_affect.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/sysvar_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69999 +/- ##
================================================
- Coverage 76.3233% 73.9297% -2.3937%
================================================
Files 2041 2058 +17
Lines 559759 579112 +19353
================================================
+ Hits 427227 428136 +909
- Misses 131631 150619 +18988
+ Partials 901 357 -544
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
e37f6a9 to
9b529fa
Compare
| @@ -0,0 +1,213 @@ | |||
| # TiDB FULL OUTER JOIN Development Notes | |||
There was a problem hiding this comment.
The place is against the current file structure.
There was a problem hiding this comment.
ok, i move it to docs/agents/executor/fullouter_join_dev_note.md
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/agents/executor/fullouter_join_dev_note.md`:
- Around line 51-62: The Feature Gate section should explicitly state that Step
1 only registers tidb_enable_full_outer_join and does not consult it during
planning: PlanBuilder still returns ErrNotSupportedYet for ast.FullJoin
regardless of the variable value. Clarify that the gate begins affecting
planning in Step 2, while neither ON nor OFF changes parsing or enables
execution in Step 1.
🪄 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 Plus
Run ID: 3f9010ea-83af-466d-bbef-40e84ac60a7f
📒 Files selected for processing (1)
docs/agents/executor/fullouter_join_dev_note.md
| ### Feature Gate | ||
|
|
||
| The feature is guarded by `tidb_enable_full_outer_join`. | ||
|
|
||
| The default is `OFF` because the feature changes planner and executor behavior | ||
| across several join paths. The gate allows the implementation to land in staged | ||
| PRs without making the syntax accidentally executable before root semantics are | ||
| complete. | ||
|
|
||
| In Step 1, `FULL OUTER JOIN` still returns `ErrNotSupportedYet` even if the | ||
| variable is set to `ON`. This is intentional: Step 1 only makes the syntax | ||
| recognizable and prevents silent fallback to another join type. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify that the gate is not active in Step 1.
PlanBuilder currently returns ErrNotSupportedYet for ast.FullJoin regardless of tidb_enable_full_outer_join. Please state explicitly that the sysvar is registered in Step 1 but only affects planning once Step 2 implementation lands; otherwise readers may expect ON to enable execution or OFF to reject parsing.
🤖 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 `@docs/agents/executor/fullouter_join_dev_note.md` around lines 51 - 62, The
Feature Gate section should explicitly state that Step 1 only registers
tidb_enable_full_outer_join and does not consult it during planning: PlanBuilder
still returns ErrNotSupportedYet for ast.FullJoin regardless of the variable
value. Clarify that the gate begins affecting planning in Step 2, while neither
ON nor OFF changes parsing or enables execution in Step 1.
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gengliqi, wjhuang2016 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
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
4 participants
What problem does this PR solve?
Issue Number: ref #69998
Problem Summary:
TiDB does not support SQL standard
FULL OUTER JOIN. This PR is the first step of the staged implementation. It adds parser/sysvar preparation and a planner safety gate, while keepingFULL OUTER JOINexecution unsupported until root planner and executor semantics are implemented in a follow-up PR.What changed and how does it work?
This PR adds the first-stage groundwork for
FULL OUTER JOIN:FULL OUTER JOINin the parser and AST restore path.FULL JOINas MySQL-compatible alias syntax instead of treating it as full outer join shorthand.tidb_enable_full_outer_join, defaultOFF, as the feature gate for later implementation stages.PlanBuilderfail-fast guard so parsedFULL OUTER JOINreturnsErrNotSupportedYetinstead of silently falling through to inner join behavior.This PR intentionally does not add executable full outer join semantics. Root HashJoin v1 support and TiFlash MPP support are planned as follow-up PRs.
Check List
Tests
Test commands:
make bazel_prepare ./tools/check/failpoint-go-test.sh pkg/parser -run TestDMLStmt -count=1 ./tools/check/failpoint-go-test.sh pkg/sessionctx/variable -run TestTiDBEnableFullOuterJoin -count=1 ./tools/check/failpoint-go-test.sh pkg/planner/core -run TestFullOuterJoinSyntaxUnsupported -count=1 git diff --check HEAD /bin/bash -lc "make lint 2>&1"Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
FULL OUTER JOIN.tidb_enable_full_outer_joinsession/global setting (default OFF).FULL OUTER JOINsyntax is recognized, but planning still fails with a “not supported yet” error (even when enabled).FULL OUTER JOINbehavior and the new setting.