-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
[fix](inveted index) fix variant index #36163
Conversation
Thank you for your contribution to Apache Doris. Since 2024-03-18, the Document has been moved to doris-website. |
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
clang-tidy review says "All clean, LGTM! 👍" |
ad3cd43
to
1dde935
Compare
run buildall |
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
clang-tidy review says "All clean, LGTM! 👍" |
TeamCity be ut coverage result: |
TPC-H: Total hot run time: 42346 ms
|
TPC-DS: Total hot run time: 172357 ms
|
ClickBench: Total hot run time: 30.55 s
|
clang-tidy review says "All clean, LGTM! 👍" |
run buildall |
TeamCity be ut coverage result: |
TPC-H: Total hot run time: 39433 ms
|
TPC-DS: Total hot run time: 173001 ms
|
ClickBench: Total hot run time: 30.82 s
|
@@ -389,7 +389,7 @@ void inherit_column_attributes(const TabletColumn& source, TabletColumn& target, | |||
// add index meta | |||
TabletIndex index_info = *source_index_meta; | |||
index_info.set_escaped_escaped_index_suffix_path(target.path_info_ptr()->get_path()); | |||
const auto* target_index_meta = target_schema->get_inverted_index(target); | |||
const auto* target_index_meta = target_schema->get_inverted_index(target, false); |
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.
why set to false, and default true, add comment to explain?
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.
True: Check if the columns from the variant support inverted index
False: No need to check, just inherit directly
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.
Why not always check it?
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.
Why not always check it?
- When the variant column is created, it has a TabletIndex. The extracted column from the variant will inherit this TabletIndex.
- The variant column itself is JSONB and does not support index, no need to check.
run buildall |
clang-tidy review says "All clean, LGTM! 👍" |
TPC-H: Total hot run time: 40122 ms
|
TeamCity be ut coverage result: |
TPC-DS: Total hot run time: 170087 ms
|
ClickBench: Total hot run time: 31.08 s
|
run buildall |
clang-tidy review says "All clean, LGTM! 👍" |
TeamCity be ut coverage result: |
TPC-H: Total hot run time: 40095 ms
|
85c525b
to
0c0a2b0
Compare
run buildall |
clang-tidy review says "All clean, LGTM! 👍" |
TeamCity be ut coverage result: |
TPC-H: Total hot run time: 40168 ms
|
TPC-DS: Total hot run time: 173994 ms
|
ClickBench: Total hot run time: 30.83 s
|
0c0a2b0
to
c3c85de
Compare
run buildall |
clang-tidy review says "All clean, LGTM! 👍" |
TPC-H: Total hot run time: 39975 ms
|
TeamCity be ut coverage result: |
TPC-DS: Total hot run time: 171343 ms
|
ClickBench: Total hot run time: 30.93 s
|
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.
LGTM
Some columns from the variant do not support indexing, but they are listed in TabletIndex. If such a column can obtain the TabletIndex, the corresponding index file will not be found in copying files, creating snapshots, and uploading files to S3. When the variant column is created, it has a TabletIndex. The extracted column from the variant will inherit this TabletIndex. If the column extracted from the variant does not support index, it should not get TabletIndex.
Some columns from the variant do not support indexing, but they are listed in TabletIndex. If such a column can obtain the TabletIndex, the corresponding index file will not be found in copying files, creating snapshots, and uploading files to S3. When the variant column is created, it has a TabletIndex. The extracted column from the variant will inherit this TabletIndex. If the column extracted from the variant does not support index, it should not get TabletIndex.
When reading from segment, the schema type is variant, if we check type valid in `get_inverted_index`, the result should always return nullptr(since variant type it self does not support inverted index), but the actual storage could be `string` or etc.So we should ignore the type check and return the correct inverted index iterators introduced by #36163
When reading from segment, the schema type is variant, if we check type valid in `get_inverted_index`, the result should always return nullptr(since variant type it self does not support inverted index), but the actual storage could be `string` or etc.So we should ignore the type check and return the correct inverted index iterators introduced by apache#36163
Proposed changes
Some columns from the variant do not support indexing, but they are listed in TabletIndex. If such a column can obtain the TabletIndex, the corresponding index file will not be found in copying files, creating snapshots, and uploading files to S3.
When the variant column is created, it has a TabletIndex. The extracted column from the variant will inherit this TabletIndex.
If the column extracted from the variant does not support index, it should not get TabletIndex.
Issue Number: close #xxx