ddl: enable modify column on partitioned table#40634
Conversation
|
[REVIEW NOTIFICATION] This pull request has not been approved. To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. DetailsReviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
release-note-none
ti-chi-bot
added
the
needs-rebase
|
@mjonss: PR needs rebase. DetailsInstructions 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/test-infra repository. |
ti-chi-bot
Bot
added
size/L
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #40634 +/- ##
================================================
+ Coverage 72.6924% 75.7624% +3.0700%
================================================
Files 1866 1889 +23
Lines 506215 524971 +18756
================================================
+ Hits 367980 397731 +29751
+ Misses 115858 103618 -12240
- Partials 22377 23622 +1245
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
size/XL
There was a problem hiding this comment.
Pull Request Overview
This PR enables support for modifying columns on partitioned tables, a feature that was previously blocked. The implementation adds validation logic to ensure column modifications on partitioning columns are safe and don't affect partition distribution.
- Removes the blanket restriction on modifying columns in partitioned tables
- Adds pre-check and post-check validation for partitioning column modifications
- Updates tests to reflect the new capability and adjusted error messages
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/ddl/modify_column.go | Moved partition table restriction to conditional validation; added preCheckPartitionModifiableColumn and postCheckPartitionModifiableColumn functions with extracted logic from GetModifiableColumnJob |
| pkg/ddl/column.go | Added support for ActionModifyColumn in partition table handling logic; removed TODO comments blocking partition table support |
| pkg/ddl/index.go | Added logic to handle partition iteration for modify column operations (case 5) |
| pkg/ddl/tests/partition/db_partition_test.go | Removed test skip; updated error message expectations; added new comprehensive test TestAlterModifyColumnOnPartitionedTable and regression test TestIssue57780 |
| pkg/ddl/tests/partition/multi_domain_test.go | Simplified expected error messages to remove key-specific suffix variations |
| pkg/ddl/tests/fail/fail_db_test.go | Updated expected error message for partition table modification |
| pkg/ddl/index_modify_test.go | Changed test from expecting error to expecting successful execution |
| tests/realtikvtest/addindextest3/temp_index_test.go | Enhanced assertion message with more diagnostic information |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/retest |
|
@mjonss: Cannot trigger testing until a trusted user reviews the PR and leaves an 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. |
ti-chi-bot
Bot
added
needs-1-more-lgtm
| // TODO: update the partitioning columns with new names if column is renamed | ||
| // Would be an extension from MySQL which does not support it. |
There was a problem hiding this comment.
Prefer keeping this comment, so later we can easily understand why we don't support changing column name.
There was a problem hiding this comment.
I removed it since I an not aware that you can rename during MODIFY COLUMN? I can still keep it, but is it the right place for the comment?
There was a problem hiding this comment.
I just saw we still check the name in checkPartitionColumnModifiable, and return error if the name has changed, but it's OK to remove it.
| " PARTITION `pMax` VALUES LESS THAN (MAXVALUE))")) | ||
| tk.MustExec(`set session tidb_enable_fast_table_check = off`) | ||
| tk.MustExec(`admin check table t`) | ||
| tk.MustExec(`alter table t modify b varchar(200) charset latin1`) |
There was a problem hiding this comment.
Why can this line execute success, I think we check and require the charset must be same at https://github.com/pingcap/tidb/pull/40634/files#diff-d6c304fd538040182d3a53c3f82abff03e074b59725b87ab711779d00f91222eR1562-R1563
There was a problem hiding this comment.
I think we only check the partition column, which is a here
There was a problem hiding this comment.
Oh~~, indeed. Maybe we can add some unsupported modify column cases
joechenrh
changed the title
joechenrh
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joechenrh, wjhuang2016 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 |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created to branch |
What problem does this PR solve?
Issue Number: close #38297 ref #39915 close #57780
Similar to #39922
Problem Summary:
There were an issue (#39915) with modify column on partitioned tables, which caused a revert. This will add that code back and include a fix (reset the PhysicaTablelD).
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.