fix

GreptimeTeam · Sep 19, 2024 · 6ae407c · 6ae407c
1 parent 5331ed4
commit 6ae407c
Show file tree

Hide file tree

Showing 13 changed files with 125 additions and 72 deletions.
diff --git a/src/api/src/v1/column_def.rs b/src/api/src/v1/column_def.rs
@@ -148,7 +148,7 @@ mod tests {
         assert!(options.is_none());
 
         let schema = ColumnSchema::new("test", ConcreteDataType::string_datatype(), true)
-            .with_fulltext_options(FulltextOptions {
+            .with_fulltext_options(&FulltextOptions {
                 enable: true,
                 analyzer: FulltextAnalyzer::English,
                 case_sensitive: false,

diff --git a/src/datatypes/src/error.rs b/src/datatypes/src/error.rs
@@ -205,6 +205,13 @@ pub enum Error {
         #[snafu(implicit)]
         location: Location,
     },
+
+    #[snafu(display("Invalid data type {} for fulltext", data_type))]
+    InvalidFulltextDataType {
+        data_type: String,
+        #[snafu(implicit)]
+        location: Location,
+    },
 }
 
 impl ErrorExt for Error {
@@ -222,7 +229,8 @@ impl ErrorExt for Error {
             | DefaultValueType { .. }
             | DuplicateMeta { .. }
             | InvalidTimestampPrecision { .. }
-            | InvalidPrecisionOrScale { .. } => StatusCode::InvalidArguments,
+            | InvalidPrecisionOrScale { .. }
+            | InvalidFulltextDataType { .. } => StatusCode::InvalidArguments,
 
             ValueExceedsPrecision { .. }
             | CastType { .. }

diff --git a/src/datatypes/src/schema.rs b/src/datatypes/src/schema.rs
@@ -320,7 +320,6 @@ fn validate_timestamp_index(column_schemas: &[ColumnSchema], timestamp_index: us
 /// - `None` if the options are invalid or do not contain full-text related keys.
 pub fn parse_fulltext_options(options: &HashMap<String, String>) -> Option<FulltextOptions> {
     let mut fulltext = FulltextOptions::default();
-    let mut is_fulltext_key_exist = false;
 
     // Check and parse the "analyzer" option
     if let Some(analyzer) = options.get(COLUMN_FULLTEXT_OPT_KEY_ANALYZER) {
@@ -338,7 +337,6 @@ pub fn parse_fulltext_options(options: &HashMap<String, String>) -> Option<Fullt
                 return None;
             }
         }
-        is_fulltext_key_exist = true;
     }
 
     // Check and parse the "case_sensitive" option
@@ -357,11 +355,10 @@ pub fn parse_fulltext_options(options: &HashMap<String, String>) -> Option<Fullt
                 return None;
             }
         }
-        is_fulltext_key_exist = true;
     }
 
     // If any fulltext-related key exists, return the constructed FulltextOptions
-    if is_fulltext_key_exist {
+    if fulltext.enable {
         Some(fulltext)
     } else {
         // Return None if no valid fulltext keys are found

diff --git a/src/datatypes/src/schema/column_schema.rs b/src/datatypes/src/schema/column_schema.rs
@@ -255,18 +255,24 @@ impl ColumnSchema {
         }
     }
 
-    pub fn with_fulltext_options(mut self, options: FulltextOptions) -> Result<Self> {
+    pub fn with_fulltext_options(mut self, options: &FulltextOptions) -> Result<Self> {
         self.set_fulltext_options(options)?;
         Ok(self)
     }
 
-    pub fn set_fulltext_options(&mut self, options: FulltextOptions) -> Result<()> {
-        if self.data_type == ConcreteDataType::string_datatype() {
-            self.metadata.insert(
-                FULLTEXT_KEY.to_string(),
-                serde_json::to_string(&options).context(error::SerializeSnafu)?,
-            );
+    pub fn set_fulltext_options(&mut self, options: &FulltextOptions) -> Result<()> {
+        if self.data_type != ConcreteDataType::string_datatype() {
+            return error::InvalidFulltextDataTypeSnafu {
+                data_type: self.data_type.to_string(),
+            }
+            .fail();
         }
+
+        self.metadata.insert(
+            FULLTEXT_KEY.to_string(),
+            serde_json::to_string(options).context(error::SerializeSnafu)?,
+        );
+
         Ok(())
     }
 }

diff --git a/src/mito2/src/sst/index.rs b/src/mito2/src/sst/index.rs
@@ -349,7 +349,7 @@ mod tests {
         if with_fulltext {
             let column_schema =
                 ColumnSchema::new("text", ConcreteDataType::string_datatype(), true)
-                    .with_fulltext_options(FulltextOptions {
+                    .with_fulltext_options(&FulltextOptions {
                         enable: true,
                         ..Default::default()
                     })

diff --git a/src/mito2/src/sst/index/fulltext_index/creator.rs b/src/mito2/src/sst/index/fulltext_index/creator.rs
@@ -329,7 +329,7 @@ mod tests {
                     ConcreteDataType::string_datatype(),
                     true,
                 )
-                .with_fulltext_options(FulltextOptions {
+                .with_fulltext_options(&FulltextOptions {
                     enable: true,
                     analyzer: FulltextAnalyzer::English,
                     case_sensitive: true,
@@ -344,7 +344,7 @@ mod tests {
                     ConcreteDataType::string_datatype(),
                     true,
                 )
-                .with_fulltext_options(FulltextOptions {
+                .with_fulltext_options(&FulltextOptions {
                     enable: true,
                     analyzer: FulltextAnalyzer::English,
                     case_sensitive: false,
@@ -359,7 +359,7 @@ mod tests {
                     ConcreteDataType::string_datatype(),
                     true,
                 )
-                .with_fulltext_options(FulltextOptions {
+                .with_fulltext_options(&FulltextOptions {
                     enable: true,
                     analyzer: FulltextAnalyzer::Chinese,
                     case_sensitive: false,

diff --git a/src/query/src/sql/show_create_table.rs b/src/query/src/sql/show_create_table.rs
@@ -222,7 +222,7 @@ mod tests {
             ColumnSchema::new("cpu", ConcreteDataType::float64_datatype(), true),
             ColumnSchema::new("disk", ConcreteDataType::float32_datatype(), true),
             ColumnSchema::new("msg", ConcreteDataType::string_datatype(), true)
-                .with_fulltext_options(FulltextOptions {
+                .with_fulltext_options(&FulltextOptions {
                     enable: true,
                     ..Default::default()
                 })

diff --git a/src/sql/src/statements.rs b/src/sql/src/statements.rs
@@ -463,7 +463,7 @@ pub fn column_to_schema(
 
     if let Some(options) = column.extensions.build_fulltext_options()? {
         column_schema = column_schema
-            .with_fulltext_options(options)
+            .with_fulltext_options(&options)
             .context(SetFulltextOptionSnafu)?;
     }
 

diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs
@@ -524,9 +524,9 @@ impl RegionMetadataBuilder {
             AlterKind::DropColumns { names } => self.drop_columns(&names),
             AlterKind::ChangeColumnTypes { columns } => self.change_column_types(columns),
             AlterKind::ChangeColumnFulltext {
-                column_name: _,
+                column_name,
                 options,
-            } => self.change_column_fulltext(options)?,
+            } => self.change_column_fulltext(column_name, options)?,
         }
         Ok(self)
     }
@@ -628,22 +628,29 @@ impl RegionMetadataBuilder {
     }
 
     /// Changes column fulltext option.
-    fn change_column_fulltext(&mut self, options: HashMap<String, String>) -> Result<()> {
+    fn change_column_fulltext(
+        &mut self,
+        column_name: String,
+        options: HashMap<String, String>,
+    ) -> Result<()> {
         for column_meta in self.column_metadatas.iter_mut() {
-            let mut fulltext = if let Ok(Some(f)) = column_meta.column_schema.fulltext_options() {
-                f
-            } else {
-                FulltextOptions::default()
-            };
-
-            if let Some(f) = parse_fulltext_options(&options) {
-                fulltext = f;
-            }
+            if column_name.eq(&column_meta.column_schema.name) {
+                let mut fulltext = if let Ok(Some(f)) = column_meta.column_schema.fulltext_options()
+                {
+                    f
+                } else {
+                    FulltextOptions::default()
+                };
 
-            column_meta
-                .column_schema
-                .set_fulltext_options(fulltext)
-                .expect("set fulltext");
+                if let Some(f) = parse_fulltext_options(&options) {
+                    fulltext = f;
+                }
+
+                column_meta
+                    .column_schema
+                    .set_fulltext_options(&fulltext)
+                    .expect("set fulltext");
+            }
         }
 
         Ok(())
@@ -765,11 +772,18 @@ pub enum MetadataError {
         location: Location,
     },
 
-    #[snafu(display("Invalid fulltext options, column: {}", column_name))]
+    #[snafu(display(
+        "Invalid fulltext options, column: {}, key: {}, value: {}",
+        column_name,
+        key,
+        value
+    ))]
     InvalidFulltextOptions {
         #[snafu(implicit)]
         location: Location,
         column_name: String,
+        key: String,
+        value: String,
     },
 }
 

diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs
@@ -492,27 +492,44 @@ impl AlterKind {
             })?;
 
         // Validate the full-text options in a simplified manner
-        for (k, v) in options {
-            let value_lower = v.to_ascii_lowercase();
-            match k.to_ascii_lowercase().as_str() {
+        for (key, value) in options {
+            let value_lower = value.to_ascii_lowercase();
+            match key.to_ascii_lowercase().as_str() {
                 // Validate the "analyzer" option
                 COLUMN_FULLTEXT_OPT_KEY_ANALYZER => {
                     if !matches!(value_lower.as_str(), "english")
                         && !matches!(value_lower.as_str(), "chinese")
                     {
-                        return InvalidFulltextOptionsSnafu { column_name }.fail();
+                        return InvalidFulltextOptionsSnafu {
+                            column_name,
+                            key,
+                            value,
+                        }
+                        .fail();
                     }
                 }
                 // Validate the "case_sensitive" option
                 COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE => {
                     if !matches!(value_lower.as_str(), "true")
                         && !matches!(value_lower.as_str(), "false")
                     {
-                        return InvalidFulltextOptionsSnafu { column_name }.fail();
+                        return InvalidFulltextOptionsSnafu {
+                            column_name,
+                            key,
+                            value,
+                        }
+                        .fail();
                     }
                 }
                 // Return an error for any unknown options
-                _ => return InvalidFulltextOptionsSnafu { column_name }.fail(),
+                _ => {
+                    return InvalidFulltextOptionsSnafu {
+                        column_name,
+                        key,
+                        value,
+                    }
+                    .fail()
+                }
             }
         }
 

diff --git a/src/table/src/error.rs b/src/table/src/error.rs
@@ -138,11 +138,12 @@ pub enum Error {
         location: Location,
     },
 
-    #[snafu(display("Invalid fulltext options, column: {}", column_name))]
-    InvalidFulltextOptions {
+    #[snafu(display("column {} has invalid colum type to set fulltext", column_name))]
+    InvalidFulltextColumnType {
+        column_name: String,
+        source: datatypes::error::Error,
         #[snafu(implicit)]
         location: Location,
-        column_name: String,
     },
 }
 
@@ -160,11 +161,10 @@ impl ErrorExt for Error {
             Error::ColumnExists { .. } => StatusCode::TableColumnExists,
             Error::SchemaBuild { source, .. } => source.status_code(),
             Error::TableOperation { source } => source.status_code(),
+            Error::InvalidFulltextColumnType { source, .. } => source.status_code(),
             Error::ColumnNotExists { .. } => StatusCode::TableColumnNotFound,
             Error::Unsupported { .. } => StatusCode::Unsupported,
-            Error::ParseTableOption { .. } | Error::InvalidFulltextOptions { .. } => {
-                StatusCode::InvalidArguments
-            }
+            Error::ParseTableOption { .. } => StatusCode::InvalidArguments,
             Error::MissingTimeIndexColumn { .. } => StatusCode::IllegalState,
         }
     }

diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs
@@ -619,39 +619,50 @@ impl TableMeta {
             fulltext = f;
         }
 
-        let columns: Vec<_> = table_schema
+        let columns: Result<Vec<_>> = table_schema
             .column_schemas()
             .iter()
             .cloned()
             .map(|mut column| {
                 if column.name == *column_name {
                     column = column
                         .clone()
-                        .with_fulltext_options(fulltext.clone())
-                        .unwrap();
+                        .with_fulltext_options(&fulltext)
+                        .context(error::InvalidFulltextColumnTypeSnafu { column_name })?;
                 }
-                column
+                Ok(column)
             })
             .collect();
 
-        let mut builder = SchemaBuilder::try_from_columns(columns)
-            .with_context(|_| error::SchemaBuildSnafu {
-                msg: format!("Failed to convert column schemas into schema for table {table_name}"),
-            })?
-            .version(table_schema.version() + 1);
-
-        for (k, v) in table_schema.metadata().iter() {
-            builder = builder.add_metadata(k, v);
-        }
-        let new_schema = builder.build().with_context(|_| error::SchemaBuildSnafu {
-            msg: format!("Table {table_name} cannot change datatype with columns {column_name}"),
-        })?;
+        match columns {
+            Ok(columns) => {
+                let mut builder = SchemaBuilder::try_from_columns(columns)
+                    .with_context(|_| error::SchemaBuildSnafu {
+                        msg: format!(
+                            "Failed to convert column schemas into schema for table {table_name}"
+                        ),
+                    })?
+                    .version(table_schema.version() + 1);
+
+                for (k, v) in table_schema.metadata().iter() {
+                    builder = builder.add_metadata(k, v);
+                }
+                let new_schema = builder.build().with_context(|_| error::SchemaBuildSnafu {
+                    msg: format!(
+                        "Table {table_name} cannot change datatype with columns {column_name}"
+                    ),
+                })?;
 
-        let _ = meta_builder
-            .schema(Arc::new(new_schema))
-            .primary_key_indices(self.primary_key_indices.clone());
+                let _ = meta_builder
+                    .schema(Arc::new(new_schema))
+                    .primary_key_indices(self.primary_key_indices.clone());
 
-        Ok(meta_builder)
+                Ok(meta_builder)
+            }
+            Err(e) => {
+                return Err(e);
+            }
+        }
     }
 
     /// Split requests into different groups using column location info.