Storages: support building vector index for ColumnFileTiny (Part 2) #9546
Conversation
Signed-off-by: Lloyd-Pottiger <[email protected]>
ti-chi-bot
bot
added
release-note-none
Lloyd-Pottiger
requested review from
breezewish,
JaySon-Huang,
JinheLin and
CalvinNeo
| @@ -142,6 +142,18 @@ void VectorIndexHNSWBuilder::save(std::string_view path) const | |||
| RUNTIME_CHECK_MSG(result, "Failed to save vector index: {} path={}", result.error.what(), path); | |||
| } | |||
|
|
|||
| void VectorIndexHNSWBuilder::save(WriteBuffer & write_buf) const | |||
There was a problem hiding this comment.
saveToBuffer may be more clear. I am not sure whether the compiler will implicitly cast string_view as WriteBuffer cause by the future code changes.
Signed-off-by: Lloyd-Pottiger <[email protected]>
| if (tiny_file->hasIndex(index.index_id)) | ||
| continue; | ||
|
|
||
| any_new_index_build = true; |
There was a problem hiding this comment.
Why it happens before duplicated index check(L64-L68)?
There was a problem hiding this comment.
For example, there are file_1, file_2, file_3, and index_1, index_2, index_3, and file_1 with index_1, file_2 with index_2, file_3 with index_3.
any_new_index_build means whether the file needs to build the index, so any_new_index_build should be true for all files in this case.
indexes_to_build contains all index information that will be built in the task, so indexes_to_build should contain information of all indexes in this case.
|
|
||
| for (const auto & [col_id, index_infos] : col_indexes) | ||
| { | ||
| // Make sure the column_id is in the schema. |
There was a problem hiding this comment.
So we are sure that the schema is up-to-date here? I assume there is somewhere that we call syncSchema or something to ensure this?
There was a problem hiding this comment.
No, we do not need to guarantee this. If one file's schema is not up-to-date, we will just skip the new created index.
| while (true) | ||
| { | ||
| if (!should_proceed()) | ||
| throw Exception(ErrorCodes::ABORTED, "Index build is interrupted"); |
There was a problem hiding this comment.
Why use a exception here rather than a status code? I see it is originally like this in tiflash-cse, I just wonder if it is necessary to use a status code if the ErrorCodes::ABORTED is a regular case?
There was a problem hiding this comment.
Because the task is running in a thread pool, it is async.
dbms/src/Storages/DeltaMerge/ColumnFile/ColumnFileTinyVectorIndexWriter.cpp
Show resolved
Hide resolved
dbms/src/Storages/DeltaMerge/ColumnFile/ColumnFileTinyVectorIndexWriter.cpp
Show resolved
Hide resolved
Signed-off-by: Lloyd-Pottiger <[email protected]>
ti-chi-bot
bot
added
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: breezewish, CalvinNeo 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 |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
What problem does this PR solve?
Issue Number: ref #9600
Problem Summary:
What is changed and how it works?
This PR is part of https://github.com/tidbcloud/tiflash-cse/pull/293, which introduce the write path.
Unit tests will be included in next PR.
Check List
Tests
Side effects
Documentation
Release note