feat: Alter inverted index #5131
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
lyang24
force-pushed
the
alter_inverted_index
branch
from
0313c65 to
9772667
Compare
lyang24
force-pushed
the
alter_inverted_index
branch
3 times, most recently
from
6bc72b7 to
a9f433b
Compare
lyang24
requested review from
v0y4g3r,
MichaelScofield,
evenyag,
waynexia and
a team
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5131 +/- ##
==========================================
- Coverage 84.14% 83.88% -0.27%
==========================================
Files 1200 1181 -19
Lines 225414 221353 -4061
==========================================
- Hits 189682 185676 -4006
+ Misses 35732 35677 -55 |
There was a problem hiding this comment.
Copilot reviewed 6 out of 16 changed files in this pull request and generated no suggestions.
Files not reviewed (10)
- tests/cases/standalone/common/alter/change_col_inverted_index.result: Language not supported
- tests/cases/standalone/common/alter/change_col_inverted_index.sql: Language not supported
- src/store-api/src/metadata.rs: Evaluated as low risk
- src/common/grpc-expr/src/alter.rs: Evaluated as low risk
- Cargo.toml: Evaluated as low risk
- src/table/src/requests.rs: Evaluated as low risk
- src/table/src/metadata.rs: Evaluated as low risk
- src/store-api/src/region_request.rs: Evaluated as low risk
- src/common/meta/src/ddl/alter_table/region_request.rs: Evaluated as low risk
- src/common/meta/src/ddl/alter_table/update_metadata.rs: Evaluated as low risk
Comments skipped due to low confidence (4)
src/sql/src/parsers/alter_parser.rs:277
- The error message should clearly indicate that the parser was expecting either FULLTEXT or INVERTED INDEX. Consider changing it to 'Expected FULLTEXT or INVERTED INDEX after ALTER TABLE MODIFY COLUMN'.
self.expected(format!("{:?} OR INVERTED INDEX", Keyword::FULLTEXT).as_str(), self.parser.peek_token(),)
src/sql/src/parsers/alter_parser.rs:304
- The error message should clearly indicate that the parser was expecting either FULLTEXT or INVERTED INDEX. Consider changing it to 'Expected FULLTEXT or INVERTED INDEX after ALTER TABLE MODIFY COLUMN'.
self.expected(format!("{:?} OR INVERTED INDEX", Keyword::FULLTEXT).as_str(), self.parser.peek_token(),)
src/operator/src/expr_factory.rs:543
- [nitpick] The name 'SetIndex' is not very descriptive. Consider renaming it to something more specific, like 'SetColumnIndex'.
AlterTableOperation::SetIndex { options } => AlterTableKind::SetIndex(match options {
src/operator/src/expr_factory.rs:564
- [nitpick] The name 'UnsetIndex' is not very descriptive. Consider renaming it to something more specific, like 'UnsetColumnIndex'.
AlterTableOperation::UnsetIndex { options } => AlterTableKind::UnsetIndex(match options {
There was a problem hiding this comment.
Thanks for your nice work! Left some nit comments.
I wonder if the behaviour of changing inverted index is the same as fulltext index: disabling it would only stop constructing new indexes, and the previously contructed indexes will still take effect? @zhongzc
|
FYI: #5178 This issue is related to the PR, in case anyone might be interested in it. |
lyang24
force-pushed
the
alter_inverted_index
branch
2 times, most recently
from
24a3461 to
32c4d3b
Compare
src/table/src/metadata.rs
Outdated
| } | ||
| ); | ||
|
|
||
| let mut columns = Vec::with_capacity(table_schema.column_schemas().len()); |
There was a problem hiding this comment.
Compatibility needs to be considered. If all column schemas do not have an inverted key set, that is, schemas.all(!has_inverted_index_key), then columns in the primary key will be inverted indexed.
See:
greptimedb/src/store-api/src/metadata.rs
Lines 333 to 347 in bc2f05d
There was a problem hiding this comment.
thank you, I am trying to see what would go wrong here.
Are we worried about the case when remove invert index and accidentally build inverted index on primary keys? when we alter table remove inverted index it will set the inverted index value to false in column metadata - i think that could prevent the case as is_inverted_indexed check if the value is true.
There was a problem hiding this comment.
Let me try to explain this.
When the condition schemas.all(!has_inverted_index_key) is met, the columns in the primary key implicitly have a shadow attribute: is_inverted_index. However, this attribute is not observable from the column schema.
Once SET | UNSET INVERTED INDEX disrupts the condition schemas.all(!has_inverted_index_key), we need to materialize the shadow attribute.
There was a problem hiding this comment.
My explanation might have become more abstract.
In practice, when we set an inverted index for a schema that previously had no columns with an inverted index set, we need to reset inverted_indexed=true for the columns in the primary key.
There was a problem hiding this comment.
I see the primary key will be inverted index if there are no inverted index on the table, so when we add a new inverted index we need to set the inverted index = true for primary keys as well.
Yes, we can think that alter index only modifies the column schema, leaving other things to the engine. |
lyang24
force-pushed
the
alter_inverted_index
branch
from
32c4d3b to
61abba3
Compare
lyang24
force-pushed
the
alter_inverted_index
branch
from
61abba3 to
184b0d2
Compare
|
|
||
| Affected Rows: 0 | ||
|
|
||
| SHOW INDEX FROM test_pk; |
There was a problem hiding this comment.
not sure if this is the result we are expecting, nothing changes when we set/ unset inverted index on primary keys that comes with inverted index.
There was a problem hiding this comment.
The table has two primary keys with no explicit inverted index, so is_inverted_indexed and has_inverted_index_key for primary keys will return false. So that SHOW INDEX finds no inverted index in the table and add primary keys into the inverted index sets, see:
Seems we need to set inverted_indexed=true for column bar when we unset inverted index for column foo to get the expected result. When it comes to unset inverted index in all columns, seems it's impossible? cc @zhongzc
There was a problem hiding this comment.
Seems we need to set inverted_indexed=true for column bar when we unset inverted index for column foo to get the expected result.
thanks for pointing out the logic, i had a follow up when we remove foo in the next iteration there will be no inverted index on the table - do we expecting primary key added as inverted index again?
There was a problem hiding this comment.
In this case, we expect all primary key shows no inverted index in the query, ref #5131 (comment):
CREATE TABLE test_pk (..., foo STRING, bar INT, PRIMARY KEY (foo, bar));
SHOW INDEX; // inverted indexes: [foo, bar]
UNSET foo;
SHOW INDEX; // inverted indexes: [bar]
UNSET bar;
SHOW INDEX; // inverted indexes: []
SET foo;
SHOW INDEX; // inverted indexes: [foo]
There're two cases:
- All primary key are marked non-inverted index but we implicitly build inverted index for primary keys. We should show them as inverted index columns.
- All primary key are marked non-inverted index, and it's explicitly unset by users. We don't need to show them as inverted index columns.
There was a problem hiding this comment.
thanks i missed the expected result from previous comment. I added logic to make the results converge but the logic is not straight forward :(
lyang24
force-pushed
the
alter_inverted_index
branch
from
184b0d2 to
b3c8d8b
Compare
| columns.push(new_column_schema); | ||
| } else { | ||
| let mut new_column_schema = column_schema.clone(); | ||
| new_column_schema.update_inverted_index(value); |
There was a problem hiding this comment.
nit: replace set_inverted_index and update_inverted_index with with_inverted_index(mut self, value) -> Self, and remove the key when value is false.
Then, use insert_inverted_index_placeholder instead here:
greptimedb/src/sql/src/statements.rs
Line 490 in b3c8d8b
|
great job! |
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
!!! DO NOT LEAVE THIS BLOCK EMPTY !!!
Please explain IN DETAIL what the changes are in this PR and why they are needed:
This pr enables
alter table x modify column set/ unset inverted index syntax.Checklist