-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
*: support for adding/dropping vector index | tidb-test=723d2370f1b3956d22a5ab87b1fc8b9b864ca051 #55839
*: support for adding/dropping vector index | tidb-test=723d2370f1b3956d22a5ab87b1fc8b9b864ca051 #55839
Conversation
Hi @zimulala. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/vector-index #55839 +/- ##
=========================================================
Coverage ? 57.2832%
=========================================================
Files ? 1761
Lines ? 651421
Branches ? 0
=========================================================
Hits ? 373155
Misses ? 252934
Partials ? 25332
Flags with carried forward coverage won't be shown. Click here to find out more.
|
973c8f8
to
b4380ee
Compare
978e284
to
3f94ed4
Compare
9fe0e31
to
ffc8f35
Compare
pkg/parser/model/ddl.go
Outdated
// DecodeDropIndexFinishedArgs decodes the drop index job's args when it's finished. | ||
func (job *Job) DecodeDropIndexFinishedArgs() ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are trying to define a struct for each type of DDL jobs (see job_args.go in #55854), this will be removed, FYI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, please merge master, use job version 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently merge master results in a lot of commits and code changes that are not part of this PR. I will merge master after this PR.
This "drop index" type is just adding an arg, I will not change this PR for the time being, if you change it then, I will synchronously change it.
pkg/ddl/index.go
Outdated
return ver, errors.Trace(err) | ||
} | ||
|
||
func doReorgWorkForCreateIndexOnTiFlash(w *worker, jobCtx *jobContext, t *meta.Meta, job *model.Job, tbl table.Table, allIndexInfos []*model.IndexInfo, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a separate logic to handle it. And the name 'reorg' is not suitable for it. We don't want to consider the vector index during refactoring index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
055c889
to
306e80f
Compare
306e80f
to
8ac0891
Compare
ca22b96
to
39820b7
Compare
/retest-required |
@zimulala: Cannot trigger testing until a trusted user reviews the PR and leaves an In 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For planner part rest LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest LGTM
[LGTM Timeline notifier]Timeline:
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AilinKid, tangenta, wjhuang2016 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:
Approvers can indicate their approval by writing |
384d4ed
into
pingcap:feature/vector-index
What problem does this PR solve?
Issue Number: close #55840
Problem Summary:
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.