ddl: args v2 for add/drop/rename index, include PK/vector-index #56130

joechenrh · 2024-09-18T11:11:27Z

What problem does this PR solve?

Issue Number: ref #53930

Problem Summary:

What changed and how does it work?

As title.

Most part of this PR is to make the arguments compatible with V1.

Below lists the layout of arguments used in V1, which may help to read this PR:

Add index done:
    [ []int64, []bool, []int64, []bool ] <-- [ indexIDs, ifExists, partitionIDs, isGlobals ]
Rolling back add index:
    [ []CIStr, []bool, []int64 ] <-- [ indexNames, ifExists, partitionIDs ]
Rollback add index done:
    [ []int64, []bool, []int64 ] <-- [ indexIDs, ifExists, partitionIDs ]
Drop index
    [ []CIStr, bool ] <-- [ indexName, ifExist ]
Drop index done
    [ CIStr, bool, int64, []int64, bool ] <-- [ indexName, ifExist, indexID, partitionIDs, isVector ]

RunningArgs                  Add Index ---------------------------------               Drop Index
  (FillArgs)                     |			                |                   |
                                 |		    	                |                   |
                                 |		                        |                   |
                                 v 			                v                   v
FinishedArgs             Add Index Done   Rollback done <--- Rolling back add index   Drop Index Done
(FillFinishedArgs)       -------------------------------     -----------------------------------------
                              Almost the same                       Share the first two elements,
                                                                     used in checkDropIndex

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

tiprow · 2024-09-18T11:11:42Z

Hi @joechenrh. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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.

codecov · 2024-09-18T12:07:11Z

Codecov Report

Attention: Patch coverage is 85.35262% with 81 lines in your changes missing coverage. Please review.

Project coverage is 57.5172%. Comparing base (df821b9) to head (d870599).
Report is 53 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #56130         +/-   ##
=================================================
- Coverage   73.2357%   57.5172%   -15.7186%     
=================================================
  Files          1626       1778        +152     
  Lines        448692     645899     +197207     
=================================================
+ Hits         328603     371503      +42900     
- Misses        99977     249121     +149144     
- Partials      20112      25275       +5163

Flag	Coverage Δ
integration	`39.2984% <75.0452%> (?)`
unit	`72.6915% <88.6315%> (+0.2514%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.9478% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`61.6304% <60.0000%> (+16.1590%)`	⬆️

D3Hunter · 2024-09-29T09:58:51Z

please fix conflicts

joechenrh · 2024-09-30T06:43:38Z

/test pull-unit-test-ddlv1

tiprow · 2024-09-30T06:44:00Z

@joechenrh: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test pull-unit-test-ddlv1

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.

joechenrh · 2024-10-11T06:27:36Z

/retest

joechenrh · 2024-10-11T06:27:58Z

/test pull-unit-test-ddlv1

tiprow · 2024-10-11T06:28:18Z

@joechenrh: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow
/test fast_test_tiprow_ddlargsv1
/test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

fast_test_tiprow
tidb_parser_test

In response to this:

/test pull-unit-test-ddlv1

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.

D3Hunter

rest lgtm

D3Hunter · 2024-10-11T07:43:40Z

pkg/meta/model/job_args.go

 	IndexArgs []*IndexArg `json:"index_args,omitempty"`

 	// Belows are used for finished args.
+	// For dropping index, len(IndexIDs) is 1.
+	// While during rolling back add indexes, IndexIDs stores the partition IDs of the corresponding table.


too bad 🙃

too bad 🙃

You mean the comment, or this field? 🤔

Maybe I need to rename these two fields. They are confusing indeed.

IndexIDs stores the partition IDs of the corresponding table.

I mean this.

Maybe I need to rename these two fields. They are confusing indeed.

if the change is large, you can write a TODO and do it in other pr, this one too large

D3Hunter · 2024-10-11T07:54:56Z

v1 pass https://do.pingcap.net/jenkins/blue/organizations/jenkins/pingcap%2Ftidb%2Fpull_unit_test_ddlv1/detail/pull_unit_test_ddlv1/13/pipeline

joechenrh · 2024-10-11T12:24:02Z

pkg/ddl/delete_range.go

-		ifExists := make([]bool, 1)
-		isGlobal := make([]bool, 0, 1)
-		var partitionIDs []int64
-		if err := job.DecodeArgs(&allIndexIDs[0], &ifExists[0], &partitionIDs); err != nil {


The first decode is useless, since the first two elements are slice after #47135.
So in the new logic, we only decode them using slice.

zimulala · 2024-10-11T11:51:17Z

pkg/meta/model/job_args.go

+		isVector := false
+
+		// See getFinishedArgsV1 for why we may need to decode twice.
+		if err := job.DecodeArgs(&indexNames, &ifExists, &indexIDs, &isVector); err != nil {


Why isn't partitionIDs field here?

The partitionIDs are stored in IndexIDs fields for rolling back add index/primary key currently.
~~And I plan to change this confusing code in another pr. 😢~~
Updated

lance6716

/hold

feel free to unhold after check my comments

pkg/ddl/index.go

pkg/meta/model/job_args.go

ti-chi-bot · 2024-10-12T02:38:59Z

[LGTM Timeline notifier]

Timeline:

2024-10-11 07:54:31.994744856 +0000 UTC m=+1205427.414957868: ☑️ agreed by D3Hunter.
2024-10-12 02:38:58.54791669 +0000 UTC m=+65935.696826521: ☑️ agreed by lance6716.

ti-chi-bot · 2024-10-12T05:38:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, lance6716, Leavrth

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~br/OWNERS~~ [Leavrth]
~~pkg/ddl/OWNERS~~ [D3Hunter,lance6716]
~~pkg/meta/OWNERS~~ [D3Hunter,lance6716]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

joechenrh · 2024-10-12T05:46:46Z

/test pull-unit-test-ddlv1

tiprow · 2024-10-12T05:47:11Z

@joechenrh: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow
/test fast_test_tiprow_ddlargsv1
/test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

fast_test_tiprow
tidb_parser_test

In response to this:

/test pull-unit-test-ddlv1

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.

D3Hunter · 2024-10-12T06:01:15Z

/unhold

D3Hunter · 2024-10-12T11:44:55Z

/retest

joechenrh added 6 commits September 14, 2024 14:33

Add args for add/drop index/primary key

5cc608a

Update

8518713

Update code

265db55

Update code

ea07abe

Fix test

b475261

Merge branch 'master' into addargs-indexes

bc0b14f

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 18, 2024

joechenrh added 6 commits September 19, 2024 11:00

Fix bazel and add tests

2a28718

Fix test

3ffe864

Update decode for v1

c97dd4e

Merge branch 'master' into addargs-indexes

9ca24d4

Fix test

bd046aa

Add some comments

dc1642c

joechenrh force-pushed the addargs-indexes branch 2 times, most recently from 803f379 to dc1642c Compare September 20, 2024 05:50

joechenrh added 2 commits September 23, 2024 18:08

Some refactor

cdd9d74

Add argument for rename index

69fffec

joechenrh changed the title ~~ddl: args v2 for add/drop index/primary key~~ ddl: args v2 for add/drop/rename index/primary key Sep 23, 2024

D3Hunter mentioned this pull request Sep 23, 2024

ddl code refactor/optimize #54436

Open

54 tasks

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2024

Merge branch 'master' into addargs-indexes

a649d2d

Merge vector index

3134c5d

joechenrh changed the title ~~ddl: args v2 for add/drop/rename index/primary key~~ ddl: args v2 for add/drop/rename index/vector index/primary key Oct 8, 2024

Update

024e2bd

joechenrh added 3 commits October 10, 2024 23:23

Address comments

92df5ab

Merge three args into one

cff4182

Fix build

e1177cf

D3Hunter approved these changes Oct 11, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Oct 11, 2024

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2024

D3Hunter mentioned this pull request Oct 11, 2024

ddl: arg v2 for modify column and multi schema change #56536

Merged

13 tasks

joechenrh commented Oct 11, 2024

View reviewed changes

zimulala reviewed Oct 11, 2024

View reviewed changes

Update argument encoding and decoding.

fd08cef

lance6716 approved these changes Oct 12, 2024

View reviewed changes

pkg/ddl/index.go Outdated Show resolved Hide resolved

pkg/meta/model/job_args.go Outdated Show resolved Hide resolved

pkg/meta/model/job_args.go Outdated Show resolved Hide resolved

ti-chi-bot bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Oct 12, 2024

Leavrth approved these changes Oct 12, 2024

View reviewed changes

ti-chi-bot bot added the approved label Oct 12, 2024

Address comments

d870599

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 12, 2024

ti-chi-bot bot merged commit 89a5571 into pingcap:master Oct 12, 2024
27 checks passed

D3Hunter mentioned this pull request Oct 14, 2024

improve the usability of DDL Job.Args #53930

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: args v2 for add/drop/rename index, include PK/vector-index #56130

ddl: args v2 for add/drop/rename index, include PK/vector-index #56130

joechenrh commented Sep 18, 2024 •

edited

Loading

tiprow bot commented Sep 18, 2024

codecov bot commented Sep 18, 2024 •

edited

Loading

D3Hunter commented Sep 29, 2024

joechenrh commented Sep 30, 2024

tiprow bot commented Sep 30, 2024

joechenrh commented Oct 11, 2024

joechenrh commented Oct 11, 2024

tiprow bot commented Oct 11, 2024

D3Hunter left a comment

D3Hunter Oct 11, 2024

joechenrh Oct 11, 2024

joechenrh Oct 11, 2024

D3Hunter Oct 11, 2024

D3Hunter commented Oct 11, 2024

joechenrh Oct 11, 2024

zimulala Oct 11, 2024

joechenrh Oct 11, 2024 •

edited

Loading

lance6716 left a comment

ti-chi-bot bot commented Oct 12, 2024

ti-chi-bot bot commented Oct 12, 2024

joechenrh commented Oct 12, 2024

tiprow bot commented Oct 12, 2024

D3Hunter commented Oct 12, 2024

D3Hunter commented Oct 12, 2024

ddl: args v2 for add/drop/rename index, include PK/vector-index #56130

ddl: args v2 for add/drop/rename index, include PK/vector-index #56130

Conversation

joechenrh commented Sep 18, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Sep 18, 2024

codecov bot commented Sep 18, 2024 • edited Loading

Codecov Report

D3Hunter commented Sep 29, 2024

joechenrh commented Sep 30, 2024

tiprow bot commented Sep 30, 2024

joechenrh commented Oct 11, 2024

joechenrh commented Oct 11, 2024

tiprow bot commented Oct 11, 2024

D3Hunter left a comment

Choose a reason for hiding this comment

D3Hunter Oct 11, 2024

Choose a reason for hiding this comment

joechenrh Oct 11, 2024

Choose a reason for hiding this comment

joechenrh Oct 11, 2024

Choose a reason for hiding this comment

D3Hunter Oct 11, 2024

Choose a reason for hiding this comment

D3Hunter commented Oct 11, 2024

joechenrh Oct 11, 2024

Choose a reason for hiding this comment

zimulala Oct 11, 2024

Choose a reason for hiding this comment

joechenrh Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

lance6716 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Oct 12, 2024

[LGTM Timeline notifier]

ti-chi-bot bot commented Oct 12, 2024

joechenrh commented Oct 12, 2024

tiprow bot commented Oct 12, 2024

D3Hunter commented Oct 12, 2024

D3Hunter commented Oct 12, 2024

joechenrh commented Sep 18, 2024 •

edited

Loading

codecov bot commented Sep 18, 2024 •

edited

Loading

joechenrh Oct 11, 2024 •

edited

Loading