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
| ) -> Result<()> { | ||
| for column_meta in self.column_metadatas.iter_mut() { | ||
| if column_meta.column_schema.name == column_name { | ||
| ensure!( |
There was a problem hiding this comment.
discussion: i wonder if String is the only data type that support inverted index? what about json or other datatypes.
There was a problem hiding this comment.
Currently we only support string type. Other datatypes like json may be supported in the future, and we can adjust this correspondingly. See #5080
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.01% 83.74% -0.28%
==========================================
Files 1172 1172
Lines 218098 218485 +387
==========================================
- Hits 183239 182960 -279
- Misses 34859 35525 +666 |
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
from
a7382ea to
24a3461
Compare
lyang24
force-pushed
the
alter_inverted_index
branch
from
24a3461 to
32c4d3b
Compare
| @@ -0,0 +1,32 @@ | |||
| CREATE TABLE fox ( | |||
There was a problem hiding this comment.
Waiting for #5297 to be merged, you can check the effect here by checking the results of SHOW INDEX FROM <table>.
| ensure!( | ||
| column.data_type.is_string(), | ||
| error::InvalidColumnOptionSnafu { | ||
| column_name, | ||
| msg: "INVERTED index only supports string type", | ||
| } | ||
| ); |
There was a problem hiding this comment.
inverted index has no type restrictions
| } | ||
| ); | ||
|
|
||
| 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
Yes, we can think that alter index only modifies the column schema, leaving other things to the engine. |
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