parser: modify parser to support partial index (#63448)#68823
Conversation
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu> (cherry picked from commit 66ba465)
📝 WalkthroughWalkthroughThis PR adds parser support for partial indexes, allowing WHERE predicates in index definitions. The IndexOption AST node is extended with a Condition field and restoration logic, while the parser grammar is updated to recognize WHERE clauses and merge conditions across CREATE TABLE, CREATE INDEX, and ALTER TABLE statements. ChangesPartial Index Parsing
Sequence Diagram(s)The changes introduce a straightforward grammar extension without multi-component interactions warranting a sequence diagram. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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.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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/parser/ast/ddl.go (1)
841-848:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Accept() must visit the Condition field.
The
Accept()method does not visit theConditionfield when it is non-nil. This breaks AST traversal because visitors will never see partial index predicates, which could cause failures in query optimization, validation, or transformations.Following the pattern used elsewhere in this file (e.g.,
ColumnOption.Accept()at lines 684-698), the method must visit theConditionwhen it is not nil.🔧 Proposed fix to visit Condition field
// Accept implements Node Accept interface. func (n *IndexOption) Accept(v Visitor) (Node, bool) { newNode, skipChildren := v.Enter(n) if skipChildren { return v.Leave(newNode) } n = newNode.(*IndexOption) + if n.Condition != nil { + node, ok := n.Condition.Accept(v) + if !ok { + return n, false + } + n.Condition = node.(ExprNode) + } return v.Leave(n) }🤖 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/ddl.go` around lines 841 - 848, The IndexOption.Accept method currently skips visiting its Condition field; update IndexOption.Accept to follow the ColumnOption.Accept pattern: after calling v.Enter and casting newNode to *IndexOption, if n.Condition != nil call n.Condition.Accept(v) (handling returned node and ok), assign the potentially replaced node back to n.Condition, then continue to return v.Leave(n); ensure you propagate the skip/ok semantics the same way ColumnOption.Accept does so visitors see partial index predicates.pkg/parser/parser.y (1)
6616-6640:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLimit
WHEREto supported partial-index productions.Putting
WHEREinto the sharedIndexOptionrule makes it legal anywhereIndexOptionListis reused, so the parser now accepts unsupported forms likePRIMARY KEY (...) WHERE .... It also accepts repeated predicates (... WHERE a > 1 WHERE b > 2) and silently keeps the last one via the merge logic. This should be threaded through only the intended index-definition productions and rejected if specified more than once.Also applies to: 6696-6701
🤖 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/parser.y` around lines 6616 - 6640, The parser currently accepts and merges WHERE clauses from the shared IndexOption/IndexOptionList productions (IndexOptionList, IndexOption, Condition), enabling illegal uses like PRIMARY KEY (...) WHERE ... and repeated WHEREs; to fix this, remove handling/merging of Condition from the shared IndexOption/IndexOptionList rule and instead thread Condition only into the specific index-definition productions that allow partial indexes (move the WHERE parsing and assignment out of IndexOption/IndexOptionList into the index-definition grammar rules), and add a check in those specific productions to reject multiple WHERE clauses (or fail the parse) so repeated predicates are not silently overwritten; also apply the same change to the other occurrence around the 6696-6701 region where Condition is currently merged.
🤖 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.
Outside diff comments:
In `@pkg/parser/ast/ddl.go`:
- Around line 841-848: The IndexOption.Accept method currently skips visiting
its Condition field; update IndexOption.Accept to follow the ColumnOption.Accept
pattern: after calling v.Enter and casting newNode to *IndexOption, if
n.Condition != nil call n.Condition.Accept(v) (handling returned node and ok),
assign the potentially replaced node back to n.Condition, then continue to
return v.Leave(n); ensure you propagate the skip/ok semantics the same way
ColumnOption.Accept does so visitors see partial index predicates.
In `@pkg/parser/parser.y`:
- Around line 6616-6640: The parser currently accepts and merges WHERE clauses
from the shared IndexOption/IndexOptionList productions (IndexOptionList,
IndexOption, Condition), enabling illegal uses like PRIMARY KEY (...) WHERE ...
and repeated WHEREs; to fix this, remove handling/merging of Condition from the
shared IndexOption/IndexOptionList rule and instead thread Condition only into
the specific index-definition productions that allow partial indexes (move the
WHERE parsing and assignment out of IndexOption/IndexOptionList into the
index-definition grammar rules), and add a check in those specific productions
to reject multiple WHERE clauses (or fail the parse) so repeated predicates are
not silently overwritten; also apply the same change to the other occurrence
around the 6696-6701 region where Condition is currently merged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 50370752-5039-4ed6-827e-4f7e4d11e983
📒 Files selected for processing (4)
pkg/parser/ast/ddl.gopkg/parser/parser.gopkg/parser/parser.ypkg/parser/parser_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #68823 +/- ##
================================================
Coverage ? 55.8553%
================================================
Files ? 1824
Lines ? 663616
Branches ? 0
================================================
Hits ? 370665
Misses ? 266074
Partials ? 26877
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
[LGTM Timeline notifier]Timeline:
|
|
/retest |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bb7133, terry1purcell, winoros, wjhuang2016, 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 |
1 similar comment
|
/retest |
|
In response to a cherrypick label: base branch (release-8.5) needs to differ from target branch (release-8.5). |
(cherry picked from #63448)
What problem does this PR solve?
Issue Number: close #63447
-> #63448
#62759
#62762
Problem Summary:
What changed and how does it work?
Modify the parser to support partial index grammar
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