routing: fix rule validation, MQ dispatch and view rewriting - #6259
3AceShowHand wants to merge 20 commits into
Conversation
Replace route placeholders in one pass so placeholder-like text in source schema and table names remains literal. Add substitution and DDL routing regression coverage.
Validate route matchers for empty table sets and keep table-only routing rules out of MQ dispatch matching. Report successful table verification accurately and improve routing/configuration regression coverage.
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change updates route validation, case-sensitive matching, scope-aware DDL rewriting, exchange-partition metadata handling, bootstrap status preservation, and related API, unit, and integration tests. ChangesRouting validation and case sensitivity
DDL rewriting and exchange handling
Verification and integration
Priority: ➖ Normal Estimated code review effort: 5 (Critical) | ~90 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant SourceDDL
participant SchemaStore
participant DDLRewriter
participant Router
participant Dispatcher
SourceDDL->>SchemaStore: emit DDL event with table state
SchemaStore->>DDLRewriter: provide table metadata
DDLRewriter->>Router: resolve tables, aliases, CTEs, and scopes
Router->>DDLRewriter: return routed identifiers
DDLRewriter->>Dispatcher: deliver routed DDL
Dispatcher->>Dispatcher: update cached table information
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Out of Scope Changes checkExplanation The pull request contains changes without a demonstrated connection to issue ✨ 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. A rabbit hops through routes so wide Comment |
|
/test all |
3AceShowHand
changed the title
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
and removed
do-not-merge/needs-linked-issue
labels
|
/test all |
|
/test all |
|
/test all |
|
/retest |
|
/test all |
1 similar comment
|
/test all |
|
/retest |
Table-qualified references were only bound to tables of the current SELECT, so a correlated reference kept the source table name after routing and the rewritten statement pointed at a table that no longer exists. References are now resolved through the SELECT scope chain after the whole statement is visited: an alias, a CTE name, or an ambiguous declaration stops the search, and columns and wildcards share the same rules. The CREATE VIEW normalizer implements the same rules for the source schema; both sides cross-reference the rule set and pin it with mirrored test cases. Physical table identity now follows the changefeed's case-sensitive setting everywhere two names are compared: rule matching, statement rewriting, route conflict detection, and route admission. A case-insensitive changefeed treats `T` and `t` as one table, so two sources that map to case-different targets now conflict instead of silently writing into one downstream table. Tests cover the resolution matrix (shadowing, ambiguity, correlated, union, derived table, cross schema), case-sensitive binding, registry/admin identity, and two new table_route integration views (aliased and nested correlated).
Rewrite one DDL statement in a single AST pass: route and rename each table name where it is visited instead of extracting names first and matching them positionally. This removes the name extractor, the default-schema fill, the position indexes, and the target-count checks. A statement that routes nothing keeps its original text, as before. One corner changes: a schema-qualified reference is routed by rule and no longer requires the referenced table to appear in the same statement. Registry and admin cleanups: inline the single-caller remove(), share the conflict report, and read the case-sensitivity flag from the registry instead of duplicating it on Admin. Test and script cleanups follow: TestResolveDDL drives the public entry point with rules derived from its expected mappings, and the table_route script shares one correlated-view helper.
|
/test all |
|
/retest |
|
/retest |
| if current != nil { | ||
| expectedTableID = current.(*common.TableInfo).TableName.TableID | ||
| } | ||
| if ddl.TableInfo.TableName.TableID != expectedTableID { |
There was a problem hiding this comment.
This fixes the immediate EXCHANGE PARTITION issue, but it leaks DDL-specific semantics into Event Collector. Event Collector should not need to know which DDL types can change the logical owner of a physical table, and BlockedTables should not be used to infer a schema mutation: it represents barrier participation, not whether the dispatcher's cached TableInfo must be replaced.
Could we model the per-dispatcher state transition explicitly in DDLEvent instead? For example:
type TableStateChangeKind uint8
const (
TableStateUnchanged TableStateChangeKind = iota
TableStateUpdated
TableStateDeleted
)
type TableStateChange struct {
// The physical table ID whose dispatcher should apply this change.
PhysicalTableID int64
Kind TableStateChangeKind
After *common.TableInfo
}
type DDLEvent struct {
// Existing event-level metadata used by DDL routing, rewriting, and sinks.
TableInfo *common.TableInfo
MultipleTableInfos []*common.TableInfo
// The state transition for the table dispatcher receiving this event.
// It should be nil for the table-trigger event.
TableStateChange *TableStateChange
}
Schema Store already has most of the required abstraction in extractTableInfoFunc:
extractTableInfoFunc(
event *PersistedDDLEvent,
physicalTableID int64,
) (tableInfo *common.TableInfo, deleted bool)
Its return values already represent the three required states:
tableInfo != nil -> Updated
tableInfo == nil && !deleted -> Unchanged
tableInfo == nil && deleted -> Deleted
Instead of implementing the physical-to-logical table mapping again in individual buildDDLEventFunc implementations, fetchTableDDLEvents could attach the state change at a common boundary:
handler := allDDLHandlers[model.ActionType(rawEvent.Type)]
ddlEvent, ok, err := handler.buildDDLEventFunc(
&rawEvent,
tableFilter,
tableID,
)
if err != nil || !ok {
// existing handling
}
tableInfo, deleted :=
handler.extractTableInfoFunc(&rawEvent, tableID)
switch {
case tableInfo != nil:
ddlEvent.TableStateChange = &commonEvent.TableStateChange{
PhysicalTableID: tableID,
Kind: commonEvent.TableStateUpdated,
After: tableInfo,
}
case deleted:
ddlEvent.TableStateChange = &commonEvent.TableStateChange{
PhysicalTableID: tableID,
Kind: commonEvent.TableStateDeleted,
}
default:
ddlEvent.TableStateChange = &commonEvent.TableStateChange{
PhysicalTableID: tableID,
Kind: commonEvent.TableStateUnchanged,
}
}
This makes extractTableInfoFunc the single source of truth for both:
- updating versionedTableInfoStore; and
- describing the post-DDL state delivered to a table dispatcher.
The table-trigger event should continue carrying the event-level TableInfo and MultipleTableInfos required by routing and sinks, but it should not carry a per-table TableStateChange.
The router also needs to apply table routing to TableStateChange.After, just as it currently routes TableInfo and MultipleTableInfos, because Event Collector must cache the routed table identity:
if ddl.TableStateChange != nil &&
ddl.TableStateChange.After != nil {
ddl.TableStateChange.After, err =
router.ApplyToTableInfo(ddl.TableStateChange.After)
}
With this contract, Event Collector becomes DDL-agnostic:
d.tableInfoVersion.Store(ddl.FinishedTs)
change := ddl.TableStateChange
if change == nil {
return
}
if change.PhysicalTableID != tableSpan.TableID {
log.Error(
"table state change was delivered to the wrong dispatcher",
zap.Int64("expectedPhysicalTableID", tableSpan.TableID),
zap.Int64("actualPhysicalTableID", change.PhysicalTableID),
)
return
}
switch change.Kind {
case commonEvent.TableStateUnchanged:
return
case commonEvent.TableStateUpdated:
if change.After == nil {
log.Error("updated table state does not contain TableInfo")
return
}
d.tableInfo.Store(change.After)
case commonEvent.TableStateDeleted:
// Dispatcher removal is handled by the scheduling/barrier workflow.
return
}
Please also include TableStateChange in:
- DDLEvent marshal/unmarshal;
- routed-event cloning;
- size accounting, if applicable;
- mixed-version compatibility handling.
The tests should verify the contract at the Schema Store boundary:
ALTER TABLE -> Updated
CREATE VIEW broadcast -> Unchanged
CREATE TABLE LIKE referenced table -> Unchanged
EXCHANGE: old normal physical ID -> Updated to partition table
EXCHANGE: old partition ID -> Updated to normal table
DROP TABLE -> Deleted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai[bot]: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: Instructions 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. |
|
/test all |
- Drop TableStateDeleted: the event collector no-ops every non-Updated kind. - Remove the redundant nil TableInfo guard; the schema store only marks Updated together with the extracted table info. - Trim assertions duplicated by the DDL event round-trip and rolling upgrade tests.
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: coderabbitai[bot], lidezhu, wk989898 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 |
|
/test all |
3 participants
What problem does this PR solve?
Issue Number: close #6264
Also fixes stale table identities after EXCHANGE PARTITION, which can cause subsequent DML to use the previous table's route.
What is changed and how it works?
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
No configuration or event-format changes. Routing behavior changes for the affected cases described above. No performance benchmarks were run.
Do you need to update user documentation, design documentation or monitoring documentation?
The dispatch-rule comments now clarify how table-only routing and matcher-only rules affect MQ dispatch. No new configuration or monitoring settings are introduced.
Release note
Summary by CodeRabbit
New Features
Bug Fixes