logservice: qualify CREATE VIEW column references#5044
Conversation
|
Skipping CI for Draft Pull Request. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a shared normalizer that parses CREATE VIEW and stored SELECT, conditionally replaces the view Select with the stored Select, and qualifies unaliased table-qualified column references by resolving table sources; the persist handler now delegates to this normalizer and a new unit test verifies qualification scenarios. ChangesCREATE VIEW Column Reference Qualification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
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.
Code Review
This pull request implements automatic schema qualification for table-qualified column references in CREATE VIEW statements. It introduces a visitor-based mechanism to resolve table names to schemas by analyzing the SELECT statement's scope. While the implementation includes initial tests for cross-schema and alias scenarios, the reviewer suggested adding further test cases for ambiguous table names, subqueries, and joins to ensure the scope handling is robust against complex SQL structures.
|
/test all |
|
/check-issue-triage-complete |
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 `@logservice/schemastore/persist_storage_ddl_handlers.go`:
- Around line 708-727: qualifyColumnName currently only checks the innermost
scope (scopes[len-1]) so correlated subquery references in outer SELECTs are not
resolved; update createViewColumnQualifier.qualifyColumnName to iterate scopes
from innermost to outermost (for i := len(q.scopes)-1; i >= 0; i--), and for
each scope check alias/ambiguousTables and tableByName (using
strings.ToLower(c.Table.O)) and on first successful match set c.Schema =
ast.NewCIStr(schema) and q.changed = true, then return; preserve the existing
early-return guards (nil c, empty c.Table.O, or pre-existing c.Schema.O).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fb59976-3c8b-429f-9ed0-55b7076127ab
📒 Files selected for processing (2)
logservice/schemastore/persist_storage_ddl_handlers.gologservice/schemastore/persist_storage_test.go
f9b5ee2 to
3356eec
Compare
3356eec to
8cd9936
Compare
|
/test all |
|
/test all |
|
/test all |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lidezhu, wk989898 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 |
What problem does this PR solve?
Issue Number: close #5043
CREATE VIEWnormalization already uses TiDB's storedView.SelectStmtto recover resolved source table schemas. However, for statements such as:TiDB stores the SELECT as
SELECT orders.id FROM source_db.orders: the FROM table is schema-qualified, but the table-qualified column remains unqualified. If table routing later renamessource_db.orders, the column qualifier can keep pointing at the old table name.What is changed and how it works?
This PR normalizes CREATE VIEW stored SELECT statements further by qualifying unaliased table-qualified column references from the SELECT's own FROM scope.
For example, it rewrites:
into:
Explicit aliases are preserved, so this remains unchanged:
Same-schema CREATE VIEW statements still keep their original query unless this column-qualifier normalization is needed.
Check List
Tests
Commands run:
Also attempted:
go test ./logservice/schemastore -count=1This hit an existing TiDB disttask global metrics panic:
duplicate metrics collector registration attempted.Questions
Will it cause performance regression or break compatibility?
No expected performance regression. The extra AST pass only runs for CREATE VIEW persisted event normalization.
Compatibility impact is limited to normalizing persisted CREATE VIEW queries more completely. Existing queries without table-qualified column references keep the previous behavior.
Do you need to update user documentation, design documentation or monitoring documentation?
No.
Release note
Summary by CodeRabbit
Improvements
Tests