feat: Alter column fulltext option#4637
Closed
irenjj wants to merge 20 commits intoGreptimeTeam:mainfrom
Closed
Conversation
Contributor
|
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
|
irenjj
requested review from
a team,
MichaelScofield and
v0y4g3r
as code owners
Member
|
Hi @irenjj there are some conflicts |
zhongzc
reviewed
Sep 9, 2024
| column_name, | ||
| target_type, | ||
| if self.parser.parse_keyword(Keyword::SET) { | ||
| let _ = self.parser.parse_keyword(Keyword::FULLTEXT); |
Collaborator
There was a problem hiding this comment.
Maybe we can check the parse result to avoid receiving unexpected input
zhongzc
reviewed
Sep 9, 2024
Comment on lines
+84
to
+101
| let options = self | ||
| .parser | ||
| .parse_options(Keyword::WITH)? | ||
| .into_iter() | ||
| .map(|option: SqlOption| -> Result<(String, String)> { | ||
| let (key, value) = (option.name, option.value); | ||
| let v = match value { | ||
| Expr::Value(sqlparser::ast::Value::SingleQuotedString(v)) | ||
| | Expr::Value(sqlparser::ast::Value::DoubleQuotedString(v)) => v, | ||
| Expr::Identifier(v) => v.value, | ||
| Expr::Value(sqlparser::ast::Value::Number(v, _)) => v.to_string(), | ||
| _ => return error::InvalidTableOptionValueSnafu { key, value }.fail(), | ||
| }; | ||
| let k = key.value.to_lowercase(); | ||
| Ok((k, v)) | ||
| }) | ||
| .collect::<Result<HashMap<String, String>>>() | ||
| .unwrap(); |
Collaborator
There was a problem hiding this comment.
Can be simplied as
Suggested change
| let options = self | |
| .parser | |
| .parse_options(Keyword::WITH)? | |
| .into_iter() | |
| .map(|option: SqlOption| -> Result<(String, String)> { | |
| let (key, value) = (option.name, option.value); | |
| let v = match value { | |
| Expr::Value(sqlparser::ast::Value::SingleQuotedString(v)) | |
| | Expr::Value(sqlparser::ast::Value::DoubleQuotedString(v)) => v, | |
| Expr::Identifier(v) => v.value, | |
| Expr::Value(sqlparser::ast::Value::Number(v, _)) => v.to_string(), | |
| _ => return error::InvalidTableOptionValueSnafu { key, value }.fail(), | |
| }; | |
| let k = key.value.to_lowercase(); | |
| Ok((k, v)) | |
| }) | |
| .collect::<Result<HashMap<String, String>>>() | |
| .unwrap(); | |
| let options = self | |
| .parser | |
| .parse_options(Keyword::WITH)? | |
| .into_iter() | |
| .map(parse_option_string) | |
| .collect::<Result<HashMap<String, String>>>()?; |
zhongzc
reviewed
Sep 9, 2024
Comment on lines
+609
to
+615
| let index = table_schema | ||
| .column_index_by_name(column_name) | ||
| .with_context(|| error::ColumnNotExistsSnafu { | ||
| column_name, | ||
| table_name, | ||
| })?; | ||
| let column = &table_schema.column_schemas()[index]; |
Collaborator
There was a problem hiding this comment.
Suggested change
| let index = table_schema | |
| .column_index_by_name(column_name) | |
| .with_context(|| error::ColumnNotExistsSnafu { | |
| column_name, | |
| table_name, | |
| })?; | |
| let column = &table_schema.column_schemas()[index]; | |
| let column = &table_schema.column_schema_by_name(column_name).with_context(...); |
zhongzc
reviewed
Sep 9, 2024
Collaborator
There was a problem hiding this comment.
I propose that we only allow modify column to create a fulltext index on columns that do not already have one, rather than modifying fulltext options for existing columns.
Member
There was a problem hiding this comment.
So we can't disable the full-text index after creation?
zhongzc
reviewed
Sep 9, 2024
| } | ||
|
|
||
| pub fn set_fulltext_options(&mut self, options: FulltextOptions) -> Result<()> { | ||
| self.metadata.insert( |
Collaborator
There was a problem hiding this comment.
Maybe you can add a check whether the column type is string: self.data_type == ConcreteDataType::string_datatype(), and also change the with_fulltext_options above
WenyXu
reviewed
Sep 9, 2024
Member
WenyXu
left a comment
WenyXu
left a comment
There was a problem hiding this comment.
Nice work!
Some nitpicking:
- Add more unit tests
- Reduce duplicate code by introducing functions
- Avoid unnecessary cloning
| "multipart", | ||
| ] } | ||
| # SCRAM-SHA-512 requires https://github.com/dequbed/rsasl/pull/48, https://github.com/influxdata/rskafka/pull/247 | ||
| # SCRAM-SHA-512 requires https://github.com/dequbed/rsasl/pull/48, https://github.com/influxdata/rskafka/pull/247 |
Member
There was a problem hiding this comment.
nit: No need to change this. The comments have been removed in the latest main.
Comment on lines
+266
to
+272
| pub fn set_fulltext_options(&mut self, options: FulltextOptions) -> Result<()> { | ||
| self.metadata.insert( | ||
| FULLTEXT_KEY.to_string(), | ||
| serde_json::to_string(&options).context(error::SerializeSnafu)?, | ||
| ); | ||
| Ok(()) | ||
| } |
Member
There was a problem hiding this comment.
Prefer to reduce the duplicated code
Option 1: refactor the with_fulltext_options method
pub fn with_fulltext_options(mut self, options: FulltextOptions) -> Result<Self> {
self. set_fulltext_options(options)?;
Ok(self)
}Option 2: use the with_fulltext_options method
column_meta.column_schema = column_meta.column_schema.with_fulltext_options(fulltext)?;| _ => {} | ||
| } | ||
| } | ||
| column_meta.column_schema.set_fulltext_options(fulltext).expect("set fulltext"); |
| fulltext.enable = true; | ||
| fulltext.case_sensitive = false; | ||
| } | ||
| _ => {} |
Member
There was a problem hiding this comment.
It's better to throw an error here. What will happen if the user sets the invalid full-text options while creating a table? @zhongzc
Collaborator
There was a problem hiding this comment.
will return error
return FulltextInvalidOptionSnafu {
msg: format!("{analyzer}, expected: 'English' | 'Chinese'"),
}
.fail();
Comment on lines
+623
to
+652
| if let Some(analyzer) = options.get(COLUMN_FULLTEXT_OPT_KEY_ANALYZER) { | ||
| match analyzer.to_ascii_lowercase().as_str() { | ||
| "english" => { | ||
| fulltext.enable = true; | ||
| fulltext.analyzer = FulltextAnalyzer::English; | ||
| } | ||
| "chinese" => { | ||
| fulltext.enable = true; | ||
| fulltext.analyzer = FulltextAnalyzer::Chinese; | ||
| } | ||
| _ => { | ||
| return error::InvalidFulltextOptionsSnafu { column_name }.fail()?; | ||
| } | ||
| } | ||
| } | ||
| if let Some(case_sensitive) = options.get(COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE) { | ||
| match case_sensitive.to_ascii_lowercase().as_str() { | ||
| "true" => { | ||
| fulltext.enable = true; | ||
| fulltext.case_sensitive = true; | ||
| } | ||
| "false" => { | ||
| fulltext.enable = true; | ||
| fulltext.case_sensitive = false; | ||
| } | ||
| _ => { | ||
| return error::InvalidFulltextOptionsSnafu { column_name }.fail()?; | ||
| } | ||
| } | ||
| } |
Comment on lines
+81
to
+113
| if self.parser.parse_keyword(Keyword::SET) { | ||
| let _ = self.parser.parse_keyword(Keyword::FULLTEXT); | ||
|
|
||
| let options = self | ||
| .parser | ||
| .parse_options(Keyword::WITH)? | ||
| .into_iter() | ||
| .map(|option: SqlOption| -> Result<(String, String)> { | ||
| let (key, value) = (option.name, option.value); | ||
| let v = match value { | ||
| Expr::Value(sqlparser::ast::Value::SingleQuotedString(v)) | ||
| | Expr::Value(sqlparser::ast::Value::DoubleQuotedString(v)) => v, | ||
| Expr::Identifier(v) => v.value, | ||
| Expr::Value(sqlparser::ast::Value::Number(v, _)) => v.to_string(), | ||
| _ => return error::InvalidTableOptionValueSnafu { key, value }.fail(), | ||
| }; | ||
| let k = key.value.to_lowercase(); | ||
| Ok((k, v)) | ||
| }) | ||
| .collect::<Result<HashMap<String, String>>>() | ||
| .unwrap(); | ||
|
|
||
| AlterTableOperation::AlterColumnFulltext { | ||
| column_name, | ||
| options: options.into(), | ||
| } | ||
| } else { | ||
| let target_type = self.parser.parse_data_type()?; | ||
|
|
||
| AlterTableOperation::ChangeColumnType { | ||
| column_name, | ||
| target_type, | ||
| } |
| Ok(meta_builder) | ||
| } | ||
|
|
||
| fn change_column_fulltext( |
Member
There was a problem hiding this comment.
Do we have a way to add tests to verify it?
zhongzc
reviewed
Sep 9, 2024
Co-authored-by: Weny Xu <wenymedia@gmail.com>
Co-authored-by: Weny Xu <wenymedia@gmail.com>
WenyXu
reviewed
Sep 18, 2024
Member
|
Hi @irenjj There are some conflicts 👀 |
Co-authored-by: Weny Xu <wenymedia@gmail.com>
Co-authored-by: Weny Xu <wenymedia@gmail.com>
Collaborator
Author
yep, it's |
v0y4g3r
reviewed
Sep 18, 2024
Contributor
v0y4g3r
left a comment
v0y4g3r
left a comment
There was a problem hiding this comment.
Good job. Left some comments. I'll start the alter table option work soon, feel free to engage if you're interested in that.
| column = column | ||
| .clone() | ||
| .with_fulltext_options(fulltext.clone()) | ||
| .unwrap(); |
Contributor
There was a problem hiding this comment.
maybe add the doc for the safety of unwrap here or handle the error
Co-authored-by: Lei, HUANG <6406592+v0y4g3r@users.noreply.github.com>
github-actions
Bot
added
docs-required
Member
|
I prefer to use the struct instead of a hashmap, then we don't need to validate the hashmap everywhere. cc @evenyag @zhongzc |
Contributor
Looks good. We know the options in advance so I'd also suggest using a struct. |
Member
Collaborator
Author
Member
WenyXu
reviewed
Oct 15, 2024
|
|
||
| pub fn set_fulltext_options(&mut self, options: &FulltextOptions) -> Result<()> { | ||
| if self.data_type != ConcreteDataType::string_datatype() { | ||
| return error::InvalidFulltextDataTypeSnafu { |
Member
There was a problem hiding this comment.
So we can't disable the full-text index after creation?
Comment on lines
+100
to
+106
| }) => AlterKind::ChangeFulltext { | ||
| column_name, | ||
| options: ChangeFulltextOptions { | ||
| enable, | ||
| analyzer, | ||
| case_sensitive, | ||
| }, |
Member
There was a problem hiding this comment.
add a new function under api::v1::column_def
/// Tries to construct a `ChangeFulltextOptions` from the given `ChangeFulltext`.
pub fn try_as_change_fulltext_options(
ChangeFulltext {
column_name,
enable,
analyzer,
case_sensitive,
}: &ChangeFulltext,
) -> Result<ChangeFulltextOptions> {
let analyer = analyzer
.map(|a| Analyzer::try_from(a))
.transpose()
.context(error::DecodeProto)?;
Ok(ChangeFulltextOptions {
enable: *enable,
analyzer: analyzer,
case_sensitive: *case_sensitive,
})
}and use it like
}) => AlterKind::ChangeFulltext {
column_name,
options: column_def::try_as_change_fulltext_options(&change)?.context(...)
Comment on lines
+419
to
+471
| impl TryFrom<HashMap<String, String>> for ChangeFulltextOptions { | ||
| type Error = Error; | ||
|
|
||
| fn try_from(options: HashMap<String, String>) -> Result<Self> { | ||
| let mut fulltext = ChangeFulltextOptions::default(); | ||
|
|
||
| // Check and parse the "analyzer" option | ||
| if let Some(analyzer) = options.get(COLUMN_FULLTEXT_OPT_KEY_ANALYZER) { | ||
| match analyzer.to_ascii_lowercase().as_str() { | ||
| "english" => { | ||
| fulltext.enable = Some(true); | ||
| fulltext.analyzer = Some(0); | ||
| } | ||
| "chinese" => { | ||
| fulltext.enable = Some(true); | ||
| fulltext.analyzer = Some(1); | ||
| } | ||
| _ => { | ||
| // If the analyzer is invalid, return None to indicate failure | ||
| return error::InvalidFulltextOptionsSnafu { | ||
| key: COLUMN_FULLTEXT_OPT_KEY_ANALYZER.to_string(), | ||
| value: analyzer.to_string(), | ||
| } | ||
| .fail(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check and parse the "case_sensitive" option | ||
| if let Some(case_sensitive) = options.get(COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE) { | ||
| match case_sensitive.to_ascii_lowercase().as_str() { | ||
| "true" => { | ||
| fulltext.enable = Some(true); | ||
| fulltext.case_sensitive = Some(true); | ||
| } | ||
| "false" => { | ||
| fulltext.enable = Some(true); | ||
| fulltext.case_sensitive = Some(false); | ||
| } | ||
| _ => { | ||
| // If case sensitivity is invalid, return None | ||
| return error::InvalidFulltextOptionsSnafu { | ||
| key: COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE.to_string(), | ||
| value: case_sensitive.to_string(), | ||
| } | ||
| .fail(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(fulltext) | ||
| } | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
used by let fulltext_options: ChangeFulltextOptions = match options.try_into() {...} to extract FulltextOptions from map.
WenyXu
reviewed
Oct 16, 2024
Comment on lines
+359
to
+373
| impl TryFrom<i32> for FulltextAnalyzer { | ||
| type Error = Error; | ||
|
|
||
| fn try_from(value: i32) -> Result<Self> { | ||
| match value { | ||
| 0 => Ok(FulltextAnalyzer::English), | ||
| 1 => Ok(FulltextAnalyzer::Chinese), | ||
| _ => Err(error::InvalidFulltextOptionsSnafu { | ||
| key: "analyzer".to_string(), | ||
| value: value.to_string(), | ||
| } | ||
| .build()), | ||
| } | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Suggested change
| impl TryFrom<i32> for FulltextAnalyzer { | |
| type Error = Error; | |
| fn try_from(value: i32) -> Result<Self> { | |
| match value { | |
| 0 => Ok(FulltextAnalyzer::English), | |
| 1 => Ok(FulltextAnalyzer::Chinese), | |
| _ => Err(error::InvalidFulltextOptionsSnafu { | |
| key: "analyzer".to_string(), | |
| value: value.to_string(), | |
| } | |
| .build()), | |
| } | |
| } | |
| } |
Comment on lines
+375
to
+380
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Visit, VisitMut, Default)] | ||
| pub struct ChangeFulltextOptions { | ||
| pub enable: Option<bool>, | ||
| pub analyzer: Option<i32>, | ||
| pub case_sensitive: Option<bool>, | ||
| } |
Member
There was a problem hiding this comment.
Suggested change
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Visit, VisitMut, Default)] | |
| pub struct ChangeFulltextOptions { | |
| pub enable: Option<bool>, | |
| pub analyzer: Option<i32>, | |
| pub case_sensitive: Option<bool>, | |
| } | |
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Visit, VisitMut, Default)] | |
| pub struct ChangeFulltextOptions { | |
| pub enable: Option<bool>, | |
| pub analyzer: Option<FulltextAnalyzer>, | |
| pub case_sensitive: Option<bool>, | |
| } |
Comment on lines
+98
to
+116
| /// Tries to construct a `ChangeFulltextOptions` from the given `ChangeFulltext`. | ||
| pub fn try_as_change_fulltext_options( | ||
| ChangeFulltext { | ||
| enable, | ||
| analyzer, | ||
| case_sensitive, | ||
| .. | ||
| }: &ChangeFulltext, | ||
| ) -> Result<ChangeFulltextOptions> { | ||
| let _ = analyzer | ||
| .map(|a| Analyzer::try_from(a)) | ||
| .transpose() | ||
| .context(error::DecodeProtoSnafu)?; | ||
| Ok(ChangeFulltextOptions { | ||
| enable: *enable, | ||
| analyzer: *analyzer, | ||
| case_sensitive: *case_sensitive, | ||
| }) | ||
| } |
Member
There was a problem hiding this comment.
Suggested change
| /// Tries to construct a `ChangeFulltextOptions` from the given `ChangeFulltext`. | |
| pub fn try_as_change_fulltext_options( | |
| ChangeFulltext { | |
| enable, | |
| analyzer, | |
| case_sensitive, | |
| .. | |
| }: &ChangeFulltext, | |
| ) -> Result<ChangeFulltextOptions> { | |
| let _ = analyzer | |
| .map(|a| Analyzer::try_from(a)) | |
| .transpose() | |
| .context(error::DecodeProtoSnafu)?; | |
| Ok(ChangeFulltextOptions { | |
| enable: *enable, | |
| analyzer: *analyzer, | |
| case_sensitive: *case_sensitive, | |
| }) | |
| } | |
| /// Tries to construct a `FulltextAnalyzer` from the given `Analyzer`. | |
| pub fn try_as_fulltext_option(analyzer: Option<i32>) -> Result<Option<FulltextAnalyzer>> { | |
| let analyzer = analyzer | |
| .map(Analyzer::try_from) | |
| .transpose() | |
| .context(error::DecodeProtoSnafu)?; | |
| Ok(analyzer.map(|a| match a { | |
| Analyzer::English => FulltextAnalyzer::English, | |
| Analyzer::Chiniese => FulltextAnalyzer::Chinese, | |
| })) | |
| } | |
| /// Tries to construct a `ChangeFulltextOptions` from the given `ChangeFulltext`. | |
| pub fn try_as_change_fulltext_options( | |
| ChangeFulltext { | |
| enable, | |
| analyzer, | |
| case_sensitive, | |
| .. | |
| }: &ChangeFulltext, | |
| ) -> Result<ChangeFulltextOptions> { | |
| let analyzer = try_as_fulltext_option(*analyzer)?; | |
| Ok(ChangeFulltextOptions { | |
| enable: *enable, | |
| analyzer, | |
| case_sensitive: *case_sensitive, | |
| }) | |
| } |
Comment on lines
+506
to
+513
| alter_request::Kind::ChangeFulltext(x) => AlterKind::ChangeColumnFulltext { | ||
| column_name: x.column_name, | ||
| options: ChangeFulltextOptions { | ||
| enable: x.enable, | ||
| analyzer: x.analyzer, | ||
| case_sensitive: x.case_sensitive, | ||
| }, | ||
| }, |
Member
There was a problem hiding this comment.
Suggested change
| alter_request::Kind::ChangeFulltext(x) => AlterKind::ChangeColumnFulltext { | |
| column_name: x.column_name, | |
| options: ChangeFulltextOptions { | |
| enable: x.enable, | |
| analyzer: x.analyzer, | |
| case_sensitive: x.case_sensitive, | |
| }, | |
| }, | |
| alter_request::Kind::ChangeFulltext(x) => AlterKind::ChangeColumnFulltext { | |
| column_name: x.column_name, | |
| options: ChangeFulltextOptions { | |
| enable: x.enable, | |
| analyzer: column_def::try_as_fulltext_option(x.analyzer).context(..)?, | |
| case_sensitive: x.case_sensitive, | |
| }, | |
| }, |
Comment on lines
+382
to
+410
| impl fmt::Display for ChangeFulltextOptions { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| write!(f, "fulltext options: ")?; | ||
|
|
||
| if let Some(enable) = self.enable { | ||
| write!(f, "enable={}", enable)?; | ||
| } else { | ||
| write!(f, "enable=None")?; | ||
| } | ||
|
|
||
| if let Some(analyzer) = self.analyzer { | ||
| if analyzer == 0 { | ||
| write!(f, ", analyzer=English")?; | ||
| } else { | ||
| write!(f, ", analyzer=Chinese")?; | ||
| } | ||
| } else { | ||
| write!(f, ", analyzer=None")?; | ||
| } | ||
|
|
||
| if let Some(case_sensitive) = self.case_sensitive { | ||
| write!(f, ", case_sensitive={}", case_sensitive)?; | ||
| } else { | ||
| write!(f, ", case_sensitive=None")?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Suggested change
| impl fmt::Display for ChangeFulltextOptions { | |
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | |
| write!(f, "fulltext options: ")?; | |
| if let Some(enable) = self.enable { | |
| write!(f, "enable={}", enable)?; | |
| } else { | |
| write!(f, "enable=None")?; | |
| } | |
| if let Some(analyzer) = self.analyzer { | |
| if analyzer == 0 { | |
| write!(f, ", analyzer=English")?; | |
| } else { | |
| write!(f, ", analyzer=Chinese")?; | |
| } | |
| } else { | |
| write!(f, ", analyzer=None")?; | |
| } | |
| if let Some(case_sensitive) = self.case_sensitive { | |
| write!(f, ", case_sensitive={}", case_sensitive)?; | |
| } else { | |
| write!(f, ", case_sensitive=None")?; | |
| } | |
| Ok(()) | |
| } | |
| } | |
| impl fmt::Display for ChangeFulltextOptions { | |
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | |
| write!(f, "fulltext options: ")?; | |
| if let Some(enable) = self.enable { | |
| write!(f, "enable={}", enable)?; | |
| } else { | |
| write!(f, "enable=None")?; | |
| } | |
| if let Some(analyzer) = &self.analyzer { | |
| match analyzer { | |
| FulltextAnalyzer::English => { | |
| write!(f, ", analyzer=English")?; | |
| } | |
| FulltextAnalyzer::Chinese => { | |
| write!(f, ", analyzer=Chinese")?; | |
| } | |
| } | |
| } else { | |
| write!(f, ", analyzer=None")?; | |
| } | |
| if let Some(case_sensitive) = self.case_sensitive { | |
| write!(f, ", case_sensitive={}", case_sensitive)?; | |
| } else { | |
| write!(f, ", case_sensitive=None")?; | |
| } | |
| Ok(()) | |
| } | |
| } | |
Comment on lines
+486
to
+494
| AlterTableOperation::AlterColumnFulltext { | ||
| column_name, | ||
| options, | ||
| } => Kind::ChangeFulltext(ChangeFulltext { | ||
| column_name: column_name.value.to_string(), | ||
| enable: options.enable, | ||
| analyzer: options.analyzer, | ||
| case_sensitive: options.case_sensitive, | ||
| }), |
Member
There was a problem hiding this comment.
Suggested change
| AlterTableOperation::AlterColumnFulltext { | |
| column_name, | |
| options, | |
| } => Kind::ChangeFulltext(ChangeFulltext { | |
| column_name: column_name.value.to_string(), | |
| enable: options.enable, | |
| analyzer: options.analyzer, | |
| case_sensitive: options.case_sensitive, | |
| }), | |
| AlterTableOperation::AlterColumnFulltext { | |
| column_name, | |
| options, | |
| } => Kind::ChangeFulltext(ChangeFulltext { | |
| column_name: column_name.value.to_string(), | |
| enable: options.enable, | |
| analyzer: match options.analyzer { | |
| Some(FulltextAnalyzer::Chinese) => Some(Analyzer::Chiniese as i32), | |
| Some(FulltextAnalyzer::English) => Some(Analyzer::English as i32), | |
| None => None, | |
| }, | |
| case_sensitive: options.case_sensitive, | |
| }), |
And the found a proto typo: Chiniese
Contributor
|
Hi @irenjj , I‘ve noticed the pr is almost completed. If you're busy recently, I'd like to help to push forward this based on your great efforts :) |
Collaborator
Author
Sure, feel to continue this work, thank you! @CookiePieWw |
3 tasks
Member
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
closes: #4395
What's changed and what's your intention?
Supports alter column options like:
ALTER TABLE monitor SET COLUMN load_15 fulltext WITH(analyzer = 'Chinese', case_sensitive = 'false');Checklist