Skip to content

fix rename column: delete table with old metadata #22182

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 17, 2025

Conversation

aptend
Copy link
Contributor

@aptend aptend commented Jul 17, 2025

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #22015

What this PR does / why we need it:

  • Fix mismatched primary key values between row_ids in mo_columns when renaming a column.
  • Add BVT test cases

PR Type

Bug fix


Description

  • Fix column rename metadata synchronization issue

  • Reorder table definition updates after metadata deletion

  • Add BVT test case for rename column operation


Changes diagram

flowchart LR
  A["Old Table Metadata"] --> B["Delete Old Metadata"]
  B --> C["Update Table Definitions"]
  C --> D["Create New Metadata"]
  D --> E["Refreshed Table Definition"]
Loading

Changes walkthrough 📝

Relevant files
Bug fix
txn_table.go
Fix table definition refresh timing                                           

pkg/vm/engine/disttae/txn_table.go

  • Add RefeshTableDef method to refresh table definition
  • Reorder table definition updates to occur after metadata deletion
  • Fix timing issue in AlterTable method for rename operations
  • +14/-7   
    Tests
    alter.result
    Add rename column test results                                                     

    test/distributed/cases/ddl/alter.result

  • Add test case for column rename operation within transaction
  • Include flush and sleep operations for metadata synchronization
  • Verify table structure after rename operation
  • +39/-0   
    alter.sql
    Add rename column test case                                                           

    test/distributed/cases/ddl/alter.sql

  • Add BVT test case for rename column operation
  • Include transaction boundaries and metadata flush operations
  • Test column rename from content to new_content
  • +14/-0   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    22015 - Not compliant

    Non-compliant requirements:

    • Optimize schema change operations to avoid unnecessary data copying when possible
    • When data copying is unavoidable during schema changes, optimize implementation to prevent OOM
    • Reduce resource usage during schema change operations
    • Fine tune deep copy table data during schema change

    Requires further human verification:

    • All requirements need verification as this PR appears to be a metadata synchronization fix rather than data copying optimization

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Typo Issue

    Method name has typo - 'RefeshTableDef' should be 'RefreshTableDef'

    func (tbl *txnTable) RefeshTableDef(ctx context.Context) {
    	tbl.tableDef = nil
    Logic Change

    The reordering of table definition updates after metadata deletion may have unintended side effects and should be carefully validated

    // update table defs after deleting old table metadata
    tbl.defs = append(baseDefs, appendDef...)
    tbl.RefeshTableDef(ctx)

    @mergify mergify bot added the kind/bug Something isn't working label Jul 17, 2025
    Copy link

    You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Fix method name typo

    The method name contains a typo. "Refesh" should be "Refresh" to follow proper
    naming conventions and improve code readability.

    pkg/vm/engine/disttae/txn_table.go [1109-1112]

    -func (tbl *txnTable) RefeshTableDef(ctx context.Context) {
    +func (tbl *txnTable) RefreshTableDef(ctx context.Context) {
     	tbl.tableDef = nil
     	tbl.GetTableDef(ctx)
     }
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly identifies and fixes a typo in the new method name RefeshTableDef, improving code readability and maintainability.

    Low
    Update method call name

    Update the method call to use the corrected method name "RefreshTableDef"
    instead of the misspelled "RefeshTableDef".

    pkg/vm/engine/disttae/txn_table.go [1454]

    -	tbl.RefeshTableDef(ctx)
    +	tbl.RefreshTableDef(ctx)
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why: This suggestion correctly identifies the call site for the misspelled method RefeshTableDef, ensuring consistency if the method definition is corrected.

    Low
    • More

    @mergify mergify bot merged commit d06e36a into matrixorigin:main Jul 17, 2025
    19 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    kind/bug Something isn't working Review effort 3/5 size/S Denotes a PR that changes [10,99] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants