ddl: support modify column for multi-schema change #36040
Conversation
|
[REVIEW NOTIFICATION] This pull request has been approved by:
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. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
release-note-none
|
Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/a7fbe5be8cb83fce28c7e3f8bef66a7480d6cf74 |
ddl/index.go
Outdated
| type indexesToChange struct { | ||
| indexInfo *model.IndexInfo | ||
| offset int // column offset in idxInfo.Columns | ||
| isTemp bool // this is a temp index created by a previous modify column job. |
There was a problem hiding this comment.
IMHO it's better to add more details to the 'temp' index in the comment. The logic here is not easy to follow.
The descriptions in https://pingcap.feishu.cn/docx/doxcn53f2eGAxYPZPW01drDmLRf?from=from_lark_index_search is clear. How about putting them in the comment?
There was a problem hiding this comment.
Sure. I have updated it in the comment.
ddl/index.go
Outdated
| } | ||
| } | ||
| } | ||
| return idxInfos, offsets | ||
| // Overwrite if the index has the corresponding temporary index. For example, |
There was a problem hiding this comment.
Ditto for the comments here, please add more description on the meaning of $ when you use it in the comment("$i($a,...")
| @@ -4479,7 +4449,7 @@ func (d *ddl) getModifiableColumnJob(ctx context.Context, sctx sessionctx.Contex | |||
| Location: &model.TimeZoneLocation{Name: tzName, Offset: tzOffset}, | |||
| }, | |||
| CtxVars: []interface{}{needChangeColData}, | |||
| Args: []interface{}{&newCol, originalColName, spec.Position, modifyColumnTp, newAutoRandBits}, | |||
| Args: []interface{}{&newCol.ColumnInfo, originalColName, spec.Position, modifyColumnTp, newAutoRandBits}, | |||
There was a problem hiding this comment.
Why change this arg's type?
There was a problem hiding this comment.
Using newCol can bring some unnecessary type conversions.
It is compatible because the decoding part use *model.ColumnInfo to receive it.
ddl/index.go
Outdated
| offsets = append(offsets, i) | ||
| break | ||
| // indexesToChange is used to store the indexes that need to be changed during modifying column. | ||
| type indexesToChange struct { |
There was a problem hiding this comment.
It is an index, so indexToChange is better? Besides, would a noun make more sense? Such as changingIndex .
| defer clean() | ||
| tk := testkit.NewTestKit(t, store) | ||
| tk.MustExec("use test;") | ||
|
|
There was a problem hiding this comment.
Could we add a test for modifying the same column, like modify column a ..., modify column a ...
Co-authored-by: Lynn <zimu_xia@126.com>
ti-chi-bot
added
status/LGT2
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: 5427fcd
|
ti-chi-bot
added
the
status/can-merge
|
/run-mysql-test |
TiDB MergeCI notify🔴 Bad News! New failing [3] after this pr merged.
|
What problem does this PR solve?
Issue Number: ref #14766
Problem Summary:
This PR supports modify multiple columns in one DDL statement.
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.