parser: add LATERAL derived table syntax (MySQL WL#8652)#67076
parser: add LATERAL derived table syntax (MySQL WL#8652)#67076ti-chi-bot[bot] merged 6 commits intopingcap:masterfrom
Conversation
Add parser support for the LATERAL keyword in derived tables, following MySQL 8.0 syntax. This is parser-only; planner support follows in a separate PR. Grammar: LATERAL SubSelect TableAsName IdentListWithParenOpt - Alias is required (TableAsName, not TableAsNameOpt) - AS keyword is optional - Optional column list: LATERAL (...) AS dt(c1, c2) AST changes: - TableSource.Lateral bool flag - TableSource.ColumnNames []CIStr for column aliases - Restore() emits LATERAL keyword and column list Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note-none
|
Infrastructure Issues:
Results from Successful Agent:
Note: This review is incomplete. The posted findings are valid, but additional issues may exist that weren't caught due to the agent failures. Consider re-triggering the review once infrastructure issues are resolved. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
bot
added
the
size/XXL
|
Hi @terry1purcell. 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. |
📝 WalkthroughWalkthroughAdds LATERAL support across lexer, parser, AST restore and tests; records LATERAL and optional column lists on TableSource; planner preprocessor now rejects LATERAL derived tables as not supported. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as "Client (SQL)"
participant Lexer as "Lexer / Tokenizer"
participant Parser as "Parser (yacc)"
participant AST as "AST (TableSource)"
participant Restorer as "Restorer"
participant Planner as "Preprocessor/Planner"
Client->>Lexer: send SQL with LATERAL ...
Lexer->>Parser: emit tokens (LATERAL recognized)
Parser->>AST: build TableSource {Lateral=true, ColumnNames?}
AST->>Restorer: restore SQL (emit LATERAL and column list)
Restorer->>Client: return restored SQL
Parser->>Planner: pass AST for planning
Planner->>Planner: detect TableSource.Lateral == true
Planner-->>Client: return ErrNotSupportedYet("LATERAL derived tables")
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
📝 Coding Plan
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.11.3)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 |
|
/ok-to-test |
ti-chi-bot
bot
added
ok-to-test
![coderabbitai[bot]](https://avatars.gh.lumenfield.work/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/parser/ast/dml.go (1)
609-618: Fail fast ifColumnNamesis set without an alias.Line 609 only restores
ColumnNamesinsideAsName != ""; ifColumnNamesis populated but alias is empty, restore silently drops the column list. Prefer returning an error for this invalid AST state.Proposed guard
if asName := n.AsName.String(); asName != "" { ctx.WriteKeyWord(" AS ") ctx.WriteName(asName) if len(n.ColumnNames) > 0 { ctx.WritePlain("(") for i, col := range n.ColumnNames { if i > 0 { ctx.WritePlain(", ") } ctx.WriteName(col.String()) } ctx.WritePlain(")") } + } else if len(n.ColumnNames) > 0 { + return errors.New("column alias list requires table alias") }As per coding guidelines: "Keep error handling actionable and contextual; avoid silently swallowing errors."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/ast/dml.go` around lines 609 - 618, The restore logic currently drops n.ColumnNames when n.AsName is empty; add a guard in the Restore method surrounding the shown block to fail fast: if len(n.ColumnNames) > 0 && n.AsName == "" then return a descriptive error (e.g. "column list provided without alias") instead of silently omitting the list. Update the code path that currently writes the column list (references: n.ColumnNames, n.AsName, ctx.WritePlain, ctx.WriteName) so the guard executes before any writes and returns the error using the method's existing error return mechanism.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/parser/ast/dml.go`:
- Around line 559-562: The parser/AST exposes a Lateral flag (n.Lateral) and
restores it via ctx.WriteKeyWord, but the planner/preprocessor doesn't reject
LATERAL; add validation in the TableSource handler in preprocess.go to
explicitly error for any TableSource/derived table where x.Lateral (or the AST
node's Lateral flag) is true (e.g., check the same field the parser sets),
returning a clear "LATERAL not supported" validation error so queries with
LATERAL are rejected until planner support is implemented.
In `@pkg/parser/parser.y`:
- Line 201: The parser currently adds lateral as a reserved keyword by defining
the token lateral "LATERAL" in the reserved-keyword block, which causes unquoted
identifiers/aliases named lateral to fail at parse time; remove or comment out
the lateral "LATERAL" entry from the reserved-keyword block so lateral remains
an ordinary identifier, or instead treat it as contextual by leaving the token
out of the global reserved list and handling it as a keyword only in grammar
rules where needed (i.e., detect the IDENT "lateral" in the LATERAL-specific
grammar productions rather than reserving it globally).
---
Nitpick comments:
In `@pkg/parser/ast/dml.go`:
- Around line 609-618: The restore logic currently drops n.ColumnNames when
n.AsName is empty; add a guard in the Restore method surrounding the shown block
to fail fast: if len(n.ColumnNames) > 0 && n.AsName == "" then return a
descriptive error (e.g. "column list provided without alias") instead of
silently omitting the list. Update the code path that currently writes the
column list (references: n.ColumnNames, n.AsName, ctx.WritePlain, ctx.WriteName)
so the guard executes before any writes and returns the error using the method's
existing error return mechanism.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 50886dce-e85d-44e6-86bc-83996694a647
📒 Files selected for processing (8)
pkg/parser/BUILD.bazelpkg/parser/ast/dml.gopkg/parser/keywords.gopkg/parser/keywords_test.gopkg/parser/lateral_test.gopkg/parser/misc.gopkg/parser/parser.gopkg/parser/parser.y
…lands The parser now accepts the LATERAL keyword (from the previous commit), but the planner does not yet support it. Add a preprocessor check that returns a clear "not supported yet" error for any TableSource with Lateral=true, so users get an actionable message instead of a confusing plan-build failure. This guard will be removed when planner support for LATERAL is added. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds MySQL 8.0-style LATERAL support at the parser/AST layer for derived tables, including AST flags and SQL restoration output, while explicitly rejecting execution in the planner until full support lands.
Changes:
- Extend the grammar/keyword set to parse
LATERAL (subquery) [AS] alias [(col_list)]. - Add AST fields on
ast.TableSource(Lateral,ColumnNames) and updateRestore()to emitLATERALand the optional column list. - Add parser unit tests covering parsing + restore round-trip for
LATERALderived tables.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/planner/core/preprocess.go | Rejects LATERAL table sources during preprocessing (currently contains a critical visitor-return bug). |
| pkg/parser/parser.y | Adds LATERAL derived table production with required alias and optional column list. |
| pkg/parser/misc.go | Maps LATERAL keyword to the new token. |
| pkg/parser/keywords.go | Registers LATERAL as a reserved keyword. |
| pkg/parser/keywords_test.go | Updates keyword/reserved counts for the new keyword. |
| pkg/parser/ast/dml.go | Adds TableSource.Lateral/ColumnNames and restores them into SQL output. |
| pkg/parser/lateral_test.go | New unit tests for parsing/restoring LATERAL derived tables. |
| pkg/parser/BUILD.bazel | Adds the new test file to Bazel test targets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
terry1purcell
force-pushed
the
lateral-parser
branch
from
21e46b9 to
23ce371
Compare
Add a guard in TableSource.Restore() that returns a descriptive error if ColumnNames is non-empty but AsName is empty, instead of silently dropping the column list. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
terry1purcell
force-pushed
the
lateral-parser
branch
from
23ce371 to
e7bd465
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67076 +/- ##
================================================
- Coverage 77.7098% 77.5851% -0.1247%
================================================
Files 2016 1936 -80
Lines 552001 540124 -11877
================================================
- Hits 428959 419056 -9903
+ Misses 121300 121063 -237
+ Partials 1742 5 -1737
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/parser/ast/dml.go`:
- Around line 559-562: Before emitting SQL in TableSource.Restore(), validate
and reject unsupported AST shapes early: check fields on the TableSource
receiver (e.g., Lateral, ColumnNames, Source) and return an error if
combinations are invalid (for example Lateral==true with Source being
*TableName, ColumnNames present when Source is *TableName, or ColumnNames
without an Alias). Do this validation before branching on n.Source so that
Restore() never proceeds to write keywords or silently drop state; update the
Restore() implementation on type TableSource to perform these guards and return
clear errors for those invalid combinations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2d1a87b7-9874-4d34-8b33-b53743b0a934
📒 Files selected for processing (3)
pkg/parser/ast/dml.gopkg/planner/core/preprocess.gopkg/planner/core/preprocess_test.go
Verify that LATERAL derived tables are rejected during preprocessing with ErrNotSupportedYet, covering both comma syntax and LEFT JOIN forms. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
terry1purcell
force-pushed
the
lateral-parser
branch
from
6557730 to
866750f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/parser/ast/dml.go (1)
553-563:⚠️ Potential issue | 🟡 MinorBroaden
Lateralinvariant checks before restore.Good start, but Line 555 only blocks
*TableName.Lateralshould be rejected for any non-derived source, and Line 561 should also enforce alias-required whenLateralis true (even ifColumnNamesis empty), otherwiseRestore()can still emit invalid SQL for programmatically-built ASTs.♻️ Suggested patch
func (n *TableSource) Restore(ctx *format.RestoreCtx) error { // Validate AST invariants before emitting any SQL. - _, isTableName := n.Source.(*TableName) - if n.Lateral && isTableName { - return errors.New("LATERAL cannot be applied to a table name, only to derived tables") - } - if len(n.ColumnNames) > 0 && isTableName { - return errors.New("column alias list cannot be applied to a table name") - } + isDerivedTable := false + switch n.Source.(type) { + case *SelectStmt, *SetOprStmt: + isDerivedTable = true + } + if n.Lateral && !isDerivedTable { + return errors.New("LATERAL is only valid for derived tables") + } + if len(n.ColumnNames) > 0 && !isDerivedTable { + return errors.New("column alias list can only be applied to a derived table") + } + if n.Lateral && n.AsName.String() == "" { + return errors.New("alias required for LATERAL derived table") + } if len(n.ColumnNames) > 0 && n.AsName.String() == "" { return errors.New("column list provided without alias for derived table") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/ast/dml.go` around lines 553 - 563, The current checks only treat *TableName as invalid for LATERAL; instead, in the validation in the node handling LATERAL (check n.Lateral and n.Source), reject LATERAL for any non-derived source (i.e., require n.Source to be a *DerivedTable or equivalent derived-table node), and also require an alias whenever n.Lateral is true (even if n.ColumnNames is empty) by adding a guard that if n.Lateral && n.AsName.String() == "" return an error; update the existing conditions around n.Lateral, n.Source, n.ColumnNames and n.AsName.String() to enforce these two rules so Restore() cannot emit invalid SQL for programmatically-built ASTs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/parser/ast/dml.go`:
- Around line 553-563: The current checks only treat *TableName as invalid for
LATERAL; instead, in the validation in the node handling LATERAL (check
n.Lateral and n.Source), reject LATERAL for any non-derived source (i.e.,
require n.Source to be a *DerivedTable or equivalent derived-table node), and
also require an alias whenever n.Lateral is true (even if n.ColumnNames is
empty) by adding a guard that if n.Lateral && n.AsName.String() == "" return an
error; update the existing conditions around n.Lateral, n.Source, n.ColumnNames
and n.AsName.String() to enforce these two rules so Restore() cannot emit
invalid SQL for programmatically-built ASTs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 02508c8d-c3c9-4b10-b09e-b2128625ac30
📒 Files selected for processing (2)
pkg/parser/ast/dml.gopkg/planner/core/preprocess_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/planner/core/preprocess_test.go
![pantheon-ai[bot]](https://avatars.gh.lumenfield.work/in/2265341?s=60&v=4){kind=small}
Check Lateral flag and ColumnNames on both the original and re-parsed (round-tripped) AST so that regressions in Restore() column-list emission are caught. Also adds negative assertion for non-LATERAL cases on the round-tripped statement. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/parser/lateral_test.go (1)
188-208: Helper function correctly traverses top-level FROM clause structure.The function handles the expected AST nodes (
*ast.TableSourceand*ast.Join) that appear in FROM clauses. Note that it intentionally does not recurse intoSelectStmtorSetOprStmtsources—this means it only finds LATERAL derived tables at the current query level, not within nested subqueries. This is appropriate for the test's purpose of validating top-level LATERAL parsing.One minor observation: for test robustness, consider adding
*ast.TableNameto the switch for completeness, though it would simply return nil since table names cannot be LATERAL.Optional: Add explicit TableName case for clarity
switch n := node.(type) { case *ast.TableSource: if n.Lateral { return n } return findLateralTableSource(n.Source) case *ast.Join: if ts := findLateralTableSource(n.Left); ts != nil { return ts } return findLateralTableSource(n.Right) + case *ast.TableName: + return nil // TableName cannot be LATERAL }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/lateral_test.go` around lines 188 - 208, The test helper findLateralTableSource should explicitly handle ast.TableName to make the switch exhaustive and clearer; update the switch in findLateralTableSource to add a case for *ast.TableName that returns nil (and optionally a brief comment noting table names cannot be LATERAL) so callers see the intent when reading the function (refer to function name findLateralTableSource and the switch over node.(type)).
🤖 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/parser/lateral_test.go`:
- Around line 188-208: The test helper findLateralTableSource should explicitly
handle ast.TableName to make the switch exhaustive and clearer; update the
switch in findLateralTableSource to add a case for *ast.TableName that returns
nil (and optionally a brief comment noting table names cannot be LATERAL) so
callers see the intent when reading the function (refer to function name
findLateralTableSource and the switch over node.(type)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 012f6356-3fc6-4778-b575-ab5de335e3b7
📒 Files selected for processing (1)
pkg/parser/lateral_test.go
|
/retest |
|
/retest-required |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Benjamin2037, benmeadowcroft, fixdb, hawkingrei, tangenta, yudongusa 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 |
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
|
/retest |
1 similar comment
|
/retest |
Document the LATERAL derived table syntax support added in pingcap/tidb#67076. Includes a new page with syntax, examples, and current limitations, plus updates to mysql-compatibility.md and TOC.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
LATERAL was added as a reserved keyword in pingcap/tidb#67076. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add parser support for the LATERAL keyword in derived tables, following MySQL 8.0 syntax. This is parser-only; planner support follows in a separate PR. There is a planner change to ensure that LATERAL is correctly rejected until the planner code is delivered.
Grammar: LATERAL SubSelect TableAsName IdentListWithParenOpt
AST changes:
What problem does this PR solve?
Issue Number: ref #40328
Problem Summary:
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Tests
Bug Fixes