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
2 times, most recently
from
9fafcd4 to
6bc72b7
Compare
lyang24
force-pushed
the
alter_inverted_index
branch
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
|
|
||
| use super::create_parser::INVERTED; |
There was a problem hiding this comment.
| use super::create_parser::INVERTED; | |
| use crate::create_parser::INVERTED; |
| Analyzer::try_from(c.analyzer).context(InvalidSetFulltextOptionRequestSnafu)?, | ||
| ), | ||
| case_sensitive: c.case_sensitive, | ||
| Kind::SetIndex(o) => match o.options.unwrap() { |
There was a problem hiding this comment.
| Kind::SetIndex(o) => match o.options.unwrap() { | |
| Kind::SetIndex(o) => match o.options { | |
| Some(..) => { .. } | |
| None => { .. } | |
| } |
| }, | ||
| }, | ||
| Kind::UnsetColumnFulltext(c) => AlterKind::UnsetColumnFulltext { | ||
| column_name: c.column_name, | ||
| Kind::UnsetIndex(o) => match o.options.unwrap() { |
| | AlterKind::UnsetColumnFulltext { column_name } => { | ||
| Self::validate_column_fulltext_option(column_name, metadata)?; | ||
| AlterKind::SetIndex { options } => { | ||
| Self::validate_column_fulltext_option(options.column_name(), metadata)?; |
There was a problem hiding this comment.
Since the options is used for both kinds of index, we can change the name of the validate fn.
| Analyzer::try_from(x.analyzer).context(DecodeProtoSnafu)?, | ||
| ), | ||
| case_sensitive: x.case_sensitive, | ||
| alter_request::Kind::SetIndex(o) => match o.options.unwrap() { |
There was a problem hiding this comment.
| alter_request::Kind::SetIndex(o) => match o.options.unwrap() { | |
| alter_request::Kind::SetIndex(o) => match o.options { | |
| Some(..) => { .. } | |
| None => { .. } | |
| } |
| }, | ||
| }, | ||
| alter_request::Kind::UnsetColumnFulltext(x) => AlterKind::UnsetColumnFulltext { | ||
| column_name: x.column_name, | ||
| alter_request::Kind::UnsetIndex(o) => match o.options.unwrap() { |
| pub fn update_inverted_index(&mut self, value: bool) { | ||
| match value { | ||
| true => { | ||
| self.metadata | ||
| .insert(INVERTED_INDEX_KEY.to_string(), value.to_string()); | ||
| } | ||
|
|
||
| false => { | ||
| self.metadata.remove(INVERTED_INDEX_KEY); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
We can simply set the value since is_inverted_indexed would check the value.
| pub fn update_inverted_index(&mut self, value: bool) { | |
| match value { | |
| true => { | |
| self.metadata | |
| .insert(INVERTED_INDEX_KEY.to_string(), value.to_string()); | |
| } | |
| false => { | |
| self.metadata.remove(INVERTED_INDEX_KEY); | |
| } | |
| } | |
| } | |
| pub fn update_inverted_index(&mut self, value: bool) { | |
| self.metadata.insert(INVERTED_INDEX_KEY.to_string(), value.to_string()); | |
| } |
|
FYI: #5178 This issue is related to the PR, in case anyone might be interested in it. |
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