*: support vector index and adding/dropping vector index when doing syncTableSchema #9451

Lloyd-Pottiger · 2024-09-20T12:01:00Z

What problem does this PR solve?

Issue Number: ref #9032

Problem Summary:

What is changed and how it works?

Support the vector index from tableInfo.IndexInfo.
Support adding/dropping vector index when doing syncTableSchema.

*: support vector index and adding/dropping vector index when doing syncTableSchema

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

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

Documentation

Release note

None

sre-bot · 2024-09-20T12:01:07Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ Lloyd-Pottiger
❌ zimulala
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

JaySon-Huang · 2024-09-21T02:55:38Z

/test pull-integration-test

…yncTableSchema. (pingcap#280) Co-authored-by: JaySon <[email protected]> Co-authored-by: Lloyd-Pottiger <[email protected]>

Signed-off-by: Lloyd-Pottiger <[email protected]>

Lloyd-Pottiger · 2024-09-23T05:11:11Z

/retest

Lloyd-Pottiger · 2024-09-23T06:00:37Z

/retest

JaySon-Huang

LGTM

zimulala · 2024-09-24T11:14:37Z

dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp

    std::atomic_store(&original_table_header, std::make_shared<Block>(toEmptyBlock(original_table_columns)));
+
+    // release the lock because `applyLocalIndexChange` will try to acquire the lock


Do we need to update applyLocalIndexChange in this comment?

zimulala · 2024-09-24T11:20:36Z

dbms/src/Storages/DeltaMerge/tests/gtest_local_index_info.cpp

+        ASSERT_EQ(expect_idx2.id, idx1.index_id);
+        ASSERT_EQ(100, idx1.column_id);
+        ASSERT_NE(nullptr, idx1.index_definition);
+        ASSERT_EQ(expect_idx2.vector_index->kind, idx1.index_definition->kind);


In open-source TiDB, we remove the field of kind. We use the indexinfo.Tp instead of this kind of information. link: https://github.com/pingcap/tidb/pull/55839/files#diff-645e83502d90c4de6385256f7499dcdc234ef8b6808559e19570eb2d05c25992R54

zimulala · 2024-09-24T11:28:59Z

dbms/src/Storages/DeltaMerge/Index/IndexInfo.cpp

+{
+    LocalIndexInfosPtr index_infos = std::make_shared<LocalIndexInfos>();
+    index_infos->reserve(table_info.columns.size() + table_info.index_infos.size());
+    for (const auto & col : table_info.columns)


We needn't use this logic, because we don't support adding a vector index with a column comment.

The later PR merge the logic of function initLocalIndexInfos into generateLocalIndexInfos.

I think we can remove this for-loop after all PRs from the cse branch are merged? Or this could introduce unnecessary conflict when cherry-pick later PRs

zimulala · 2024-09-24T11:29:31Z

dbms/src/Storages/DeltaMerge/Index/IndexInfo.cpp

+    for (auto & iter : original_local_index_id_map)
+    {
+        // It means this index is create by column comments which we don't support drop index.
+        if (iter.first == DB::EmptyIndexID)


Ditto. And some tests need to update

dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp

Co-authored-by: JaySon <[email protected]>

zimulala

LGTM

ti-chi-bot · 2024-09-25T01:46:19Z

@zimulala: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

LGTM

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/test-infra repository.

ti-chi-bot · 2024-09-25T01:59:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JaySon-Huang, zanmato1984, zimulala

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:

~~OWNERS~~ [JaySon-Huang,zanmato1984]

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

ti-chi-bot · 2024-09-25T01:59:29Z

[LGTM Timeline notifier]

Timeline:

2024-09-23 06:06:25.434971515 +0000 UTC m=+1459655.175395461: ☑️ agreed by JaySon-Huang.
2024-09-25 01:59:28.531232691 +0000 UTC m=+1617638.271656629: ☑️ agreed by zanmato1984.

ti-chi-bot bot added the release-note-none Denotes a PR that doesn't merit a release note. label Sep 20, 2024

ti-chi-bot bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 20, 2024

zimulala and others added 3 commits September 23, 2024 10:30

*: support vector index and adding/dropping vector index when doing s…

d45f2d7

…yncTableSchema. (pingcap#280) Co-authored-by: JaySon <[email protected]> Co-authored-by: Lloyd-Pottiger <[email protected]>

fix

37f7569

Signed-off-by: Lloyd-Pottiger <[email protected]>

fix

f31a181

Signed-off-by: Lloyd-Pottiger <[email protected]>

Lloyd-Pottiger force-pushed the add-index-ddl branch from 18d18df to f31a181 Compare September 23, 2024 02:30

fix

90f46d8

Signed-off-by: Lloyd-Pottiger <[email protected]>

JaySon-Huang approved these changes Sep 23, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 23, 2024

zimulala reviewed Sep 24, 2024

View reviewed changes

JaySon-Huang reviewed Sep 24, 2024

View reviewed changes

dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp Outdated Show resolved Hide resolved

Update dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp

01b1983

Co-authored-by: JaySon <[email protected]>

zimulala approved these changes Sep 25, 2024

View reviewed changes

zanmato1984 approved these changes Sep 25, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 25, 2024

ti-chi-bot bot merged commit 1a781c7 into pingcap:feature/vector-index Sep 25, 2024
4 of 5 checks passed

Lloyd-Pottiger deleted the add-index-ddl branch September 27, 2024 09:19

JaySon-Huang mentioned this pull request Sep 30, 2024

*: support vector search #9486

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: support vector index and adding/dropping vector index when doing syncTableSchema #9451

*: support vector index and adding/dropping vector index when doing syncTableSchema #9451

Lloyd-Pottiger commented Sep 20, 2024

sre-bot commented Sep 20, 2024

JaySon-Huang commented Sep 21, 2024

Lloyd-Pottiger commented Sep 23, 2024

Lloyd-Pottiger commented Sep 23, 2024

JaySon-Huang left a comment

zimulala Sep 24, 2024

Lloyd-Pottiger Sep 24, 2024

zimulala Sep 24, 2024

zimulala Sep 24, 2024

JaySon-Huang Sep 24, 2024 •

edited

Loading

zimulala Sep 25, 2024

zimulala Sep 24, 2024 •

edited

Loading

JaySon-Huang Sep 24, 2024

zimulala left a comment

ti-chi-bot bot commented Sep 25, 2024

ti-chi-bot bot commented Sep 25, 2024

ti-chi-bot bot commented Sep 25, 2024

		std::atomic_store(&original_table_header, std::make_shared<Block>(toEmptyBlock(original_table_columns)));

		// release the lock because `applyLocalIndexChange` will try to acquire the lock

*: support vector index and adding/dropping vector index when doing syncTableSchema #9451

*: support vector index and adding/dropping vector index when doing syncTableSchema #9451

Conversation

Lloyd-Pottiger commented Sep 20, 2024

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

sre-bot commented Sep 20, 2024

JaySon-Huang commented Sep 21, 2024

Lloyd-Pottiger commented Sep 23, 2024

Lloyd-Pottiger commented Sep 23, 2024

JaySon-Huang left a comment

Choose a reason for hiding this comment

zimulala Sep 24, 2024

Choose a reason for hiding this comment

Lloyd-Pottiger Sep 24, 2024

Choose a reason for hiding this comment

zimulala Sep 24, 2024

Choose a reason for hiding this comment

zimulala Sep 24, 2024

Choose a reason for hiding this comment

JaySon-Huang Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

zimulala Sep 25, 2024

Choose a reason for hiding this comment

zimulala Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

JaySon-Huang Sep 24, 2024

Choose a reason for hiding this comment

zimulala left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 25, 2024

ti-chi-bot bot commented Sep 25, 2024

ti-chi-bot bot commented Sep 25, 2024

[LGTM Timeline notifier]

JaySon-Huang Sep 24, 2024 •

edited

Loading

zimulala Sep 24, 2024 •

edited

Loading