fix(plan): support MySQL-compatible chained RENAME TABLE - #24470
Conversation
The binder's buildRenameTable validated all rename pairs against the same catalog snapshot, so the standard 3-pair atomic swap pattern (RENAME TABLE a TO tmp, b TO a, tmp TO b) failed with "table already exists" on the second pair. Track rename effects across pairs during compilation: maintain a removed set and a nameMapping that records where each name moved. Source resolution consults the mapping first; destination conflict checks skip names that were already renamed away. Fixes matrixorigin#24408 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
Review: support MySQL-compatible chained RENAME TABLE
Approve
The fix correctly identifies the root cause (validation against a static catalog snapshot) and resolves it with a clean two-map approach (removed + nameMapping). Source resolution and conflict detection both respect intermediate rename state, and the invariants are maintained correctly after each pair. Existing conflict detection and double-rename errors still work as expected.
Two non-blocking observations for follow-up:
-
Cross-database renames — dstKey is built using the source schemaName rather than opt.Name.ToTableName().SchemaName, so RENAME TABLE db1.t TO db2.t chains would mismatch. This pre-exists the PR; worth a follow-up to extract the destination schema.
-
Longer-chain and no-op test cases — chains of 4+ pairs and a-to-a no-ops are untested. Not blocking, but worth adding for robustness.
LGTM for the stated scope.
Merge Queue Status
This pull request spent 1 hour 1 minute 24 seconds in the queue, including 1 hour 18 seconds running CI. Required conditions to merge
|
4 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #24408
What this PR does / why we need it:
Fix
RENAME TABLE a TO tmp, b TO a, tmp TO bfailing withtable already exists— the standard MySQL atomic swap pattern used for zero-downtime ETL refreshes.Root cause
buildRenameTablevalidated all rename pairs against the same catalog snapshot. For the 3-pair swap, pair 2 (b → a) checked whetheraexists in the catalog — it does, because pair 1 (a → tmp) hasn't executed yet. The execution phase (Scope.RenameTable) already processes pairs sequentially, but requests never reached it.Fix
Track rename effects across pairs during compilation by maintaining two maps:
removed: names that prior pairs renamed away (available as destinations)nameMapping: names that prior pairs introduced → original table metadataThree validation changes:
Verified scenarios
a→tmp, b→a, tmp→b)a→c, b→a)x→ywhen y exists)m→x, m→y)🤖 Generated with Claude Code