feat!: support alter skipping index (#5538)

* feat: support alter skipping index Signed-off-by: Ruihang Xia <[email protected]> * update test results Signed-off-by: Ruihang Xia <[email protected]> * cargo fmt Signed-off-by: Ruihang Xia <[email protected]> * update sqlness result Signed-off-by: Ruihang Xia <[email protected]> * finalize Signed-off-by: Ruihang Xia <[email protected]> --------- Signed-off-by: Ruihang Xia <[email protected]>
GreptimeTeam · Feb 14, 2025 · 7fc935c · 7fc935c
1 parent 1e6d2fb
commit 7fc935c
Show file tree

Hide file tree

Showing 18 changed files with 729 additions and 56 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -129,7 +129,7 @@ etcd-client = "0.14"
 fst = "0.4.7"
 futures = "0.3"
 futures-util = "0.3"
-greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "e2fd89fce1fe9ea0c36c85bcf447ce4bb4a84af3" }
+greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "fc09a5696608d2a0aa718cc835d5cb9c4e8e9387" }
 hex = "0.4"
 http = "1"
 humantime = "2.1"

diff --git a/src/api/src/v1/column_def.rs b/src/api/src/v1/column_def.rs
@@ -15,10 +15,10 @@
 use std::collections::HashMap;
 
 use datatypes::schema::{
-    ColumnDefaultConstraint, ColumnSchema, FulltextAnalyzer, FulltextOptions, COMMENT_KEY,
-    FULLTEXT_KEY, INVERTED_INDEX_KEY, SKIPPING_INDEX_KEY,
+    ColumnDefaultConstraint, ColumnSchema, FulltextAnalyzer, FulltextOptions, SkippingIndexType,
+    COMMENT_KEY, FULLTEXT_KEY, INVERTED_INDEX_KEY, SKIPPING_INDEX_KEY,
 };
-use greptime_proto::v1::Analyzer;
+use greptime_proto::v1::{Analyzer, SkippingIndexType as PbSkippingIndexType};
 use snafu::ResultExt;
 
 use crate::error::{self, Result};
@@ -121,6 +121,13 @@ pub fn as_fulltext_option(analyzer: Analyzer) -> FulltextAnalyzer {
     }
 }
 
+/// Tries to construct a `SkippingIndexType` from the given skipping index type.
+pub fn as_skipping_index_type(skipping_index_type: PbSkippingIndexType) -> SkippingIndexType {
+    match skipping_index_type {
+        PbSkippingIndexType::BloomFilter => SkippingIndexType::BloomFilter,
+    }
+}
+
 #[cfg(test)]
 mod tests {
 

diff --git a/src/common/grpc-expr/src/alter.rs b/src/common/grpc-expr/src/alter.rs
@@ -15,13 +15,14 @@
 use api::helper::ColumnDataTypeWrapper;
 use api::v1::add_column_location::LocationType;
 use api::v1::alter_table_expr::Kind;
-use api::v1::column_def::as_fulltext_option;
+use api::v1::column_def::{as_fulltext_option, as_skipping_index_type};
 use api::v1::{
     column_def, AddColumnLocation as Location, AlterTableExpr, Analyzer, CreateTableExpr,
     DropColumns, ModifyColumnTypes, RenameTable, SemanticType,
+    SkippingIndexType as PbSkippingIndexType,
 };
 use common_query::AddColumnLocation;
-use datatypes::schema::{ColumnSchema, FulltextOptions, RawSchema};
+use datatypes::schema::{ColumnSchema, FulltextOptions, RawSchema, SkippingIndexOptions};
 use snafu::{ensure, OptionExt, ResultExt};
 use store_api::region_request::{SetRegionOption, UnsetRegionOption};
 use table::metadata::TableId;
@@ -31,7 +32,8 @@ use table::requests::{
 };
 
 use crate::error::{
-    InvalidColumnDefSnafu, InvalidSetFulltextOptionRequestSnafu, InvalidSetTableOptionRequestSnafu,
+    InvalidColumnDefSnafu, InvalidSetFulltextOptionRequestSnafu,
+    InvalidSetSkippingIndexOptionRequestSnafu, InvalidSetTableOptionRequestSnafu,
     InvalidUnsetTableOptionRequestSnafu, MissingAlterIndexOptionSnafu, MissingFieldSnafu,
     MissingTimestampColumnSnafu, Result, UnknownLocationTypeSnafu,
 };
@@ -137,6 +139,18 @@ pub fn alter_expr_to_request(table_id: TableId, expr: AlterTableExpr) -> Result<
                         column_name: i.column_name,
                     },
                 },
+                api::v1::set_index::Options::Skipping(s) => AlterKind::SetIndex {
+                    options: SetIndexOptions::Skipping {
+                        column_name: s.column_name,
+                        options: SkippingIndexOptions {
+                            granularity: s.granularity as u32,
+                            index_type: as_skipping_index_type(
+                                PbSkippingIndexType::try_from(s.skipping_index_type)
+                                    .context(InvalidSetSkippingIndexOptionRequestSnafu)?,
+                            ),
+                        },
+                    },
+                },
             },
             None => return MissingAlterIndexOptionSnafu.fail(),
         },
@@ -152,6 +166,11 @@ pub fn alter_expr_to_request(table_id: TableId, expr: AlterTableExpr) -> Result<
                         column_name: i.column_name,
                     },
                 },
+                api::v1::unset_index::Options::Skipping(s) => AlterKind::UnsetIndex {
+                    options: UnsetIndexOptions::Skipping {
+                        column_name: s.column_name,
+                    },
+                },
             },
             None => return MissingAlterIndexOptionSnafu.fail(),
         },

diff --git a/src/common/grpc-expr/src/error.rs b/src/common/grpc-expr/src/error.rs
@@ -140,6 +140,14 @@ pub enum Error {
         error: prost::UnknownEnumValue,
     },
 
+    #[snafu(display("Invalid set skipping index option request"))]
+    InvalidSetSkippingIndexOptionRequest {
+        #[snafu(implicit)]
+        location: Location,
+        #[snafu(source)]
+        error: prost::UnknownEnumValue,
+    },
+
     #[snafu(display("Missing alter index options"))]
     MissingAlterIndexOption {
         #[snafu(implicit)]
@@ -171,6 +179,7 @@ impl ErrorExt for Error {
             Error::InvalidSetTableOptionRequest { .. }
             | Error::InvalidUnsetTableOptionRequest { .. }
             | Error::InvalidSetFulltextOptionRequest { .. }
+            | Error::InvalidSetSkippingIndexOptionRequest { .. }
             | Error::MissingAlterIndexOption { .. } => StatusCode::InvalidArguments,
         }
     }

diff --git a/src/datatypes/src/schema/column_schema.rs b/src/datatypes/src/schema/column_schema.rs
@@ -380,6 +380,11 @@ impl ColumnSchema {
         );
         Ok(())
     }
+
+    pub fn unset_skipping_options(&mut self) -> Result<()> {
+        self.metadata.remove(SKIPPING_INDEX_KEY);
+        Ok(())
+    }
 }
 
 /// Column extended type set in column schema's metadata.

diff --git a/src/operator/src/expr_helper.rs b/src/operator/src/expr_helper.rs
@@ -22,14 +22,15 @@ use api::v1::{
     set_index, unset_index, AddColumn, AddColumns, AlterDatabaseExpr, AlterTableExpr, Analyzer,
     ColumnDataType, ColumnDataTypeExtension, CreateFlowExpr, CreateTableExpr, CreateViewExpr,
     DropColumn, DropColumns, ExpireAfter, ModifyColumnType, ModifyColumnTypes, RenameTable,
-    SemanticType, SetDatabaseOptions, SetFulltext, SetIndex, SetInverted, SetTableOptions,
-    TableName, UnsetDatabaseOptions, UnsetFulltext, UnsetIndex, UnsetInverted, UnsetTableOptions,
+    SemanticType, SetDatabaseOptions, SetFulltext, SetIndex, SetInverted, SetSkipping,
+    SetTableOptions, SkippingIndexType as PbSkippingIndexType, TableName, UnsetDatabaseOptions,
+    UnsetFulltext, UnsetIndex, UnsetInverted, UnsetSkipping, UnsetTableOptions,
 };
 use common_error::ext::BoxedError;
 use common_grpc_expr::util::ColumnExpr;
 use common_time::Timezone;
 use datafusion::sql::planner::object_name_to_table_reference;
-use datatypes::schema::{ColumnSchema, FulltextAnalyzer, Schema, COMMENT_KEY};
+use datatypes::schema::{ColumnSchema, FulltextAnalyzer, Schema, SkippingIndexType, COMMENT_KEY};
 use file_engine::FileOptions;
 use query::sql::{
     check_file_to_table_schema_compatibility, file_column_schemas_to_table,
@@ -587,6 +588,19 @@ pub(crate) fn to_alter_table_expr(
                     column_name: column_name.value,
                 })),
             },
+            sql::statements::alter::SetIndexOperation::Skipping {
+                column_name,
+                options,
+            } => SetIndex {
+                options: Some(set_index::Options::Skipping(SetSkipping {
+                    column_name: column_name.value,
+                    enable: true,
+                    granularity: options.granularity as u64,
+                    skipping_index_type: match options.index_type {
+                        SkippingIndexType::BloomFilter => PbSkippingIndexType::BloomFilter.into(),
+                    },
+                })),
+            },
         }),
         AlterTableOperation::UnsetIndex { options } => AlterTableKind::UnsetIndex(match options {
             sql::statements::alter::UnsetIndexOperation::Fulltext { column_name } => UnsetIndex {
@@ -599,6 +613,11 @@ pub(crate) fn to_alter_table_expr(
                     column_name: column_name.value,
                 })),
             },
+            sql::statements::alter::UnsetIndexOperation::Skipping { column_name } => UnsetIndex {
+                options: Some(unset_index::Options::Skipping(UnsetSkipping {
+                    column_name: column_name.value,
+                })),
+            },
         }),
     };
 

diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs
@@ -25,7 +25,9 @@ use sqlparser::tokenizer::{Token, TokenWithLocation};
 use crate::error::{self, InvalidColumnOptionSnafu, Result, SetFulltextOptionSnafu};
 use crate::parser::ParserContext;
 use crate::parsers::create_parser::INVERTED;
-use crate::parsers::utils::validate_column_fulltext_create_option;
+use crate::parsers::utils::{
+    validate_column_fulltext_create_option, validate_column_skipping_index_create_option,
+};
 use crate::statements::alter::{
     AddColumn, AlterDatabase, AlterDatabaseOperation, AlterTable, AlterTableOperation,
     KeyValueOption, SetIndexOperation, UnsetIndexOperation,
@@ -241,9 +243,14 @@ impl ParserContext<'_> {
             TokenWithLocation {
                 token: Token::Word(w),
                 ..
-            } if w.keyword == Keyword::FULLTEXT => Ok(AlterTableOperation::UnsetIndex {
-                options: UnsetIndexOperation::Fulltext { column_name },
-            }),
+            } if w.keyword == Keyword::FULLTEXT => {
+                self.parser
+                    .expect_keyword(Keyword::INDEX)
+                    .context(error::SyntaxSnafu)?;
+                Ok(AlterTableOperation::UnsetIndex {
+                    options: UnsetIndexOperation::Fulltext { column_name },
+                })
+            }
 
             TokenWithLocation {
                 token: Token::Word(w),
@@ -256,8 +263,24 @@ impl ParserContext<'_> {
                     options: UnsetIndexOperation::Inverted { column_name },
                 })
             }
+
+            TokenWithLocation {
+                token: Token::Word(w),
+                ..
+            } if w.value.eq_ignore_ascii_case("SKIPPING") => {
+                self.parser
+                    .expect_keyword(Keyword::INDEX)
+                    .context(error::SyntaxSnafu)?;
+                Ok(AlterTableOperation::UnsetIndex {
+                    options: UnsetIndexOperation::Skipping { column_name },
+                })
+            }
             _ => self.expected(
-                format!("{:?} OR INVERTED INDEX", Keyword::FULLTEXT).as_str(),
+                format!(
+                    "{:?} OR INVERTED INDEX OR SKIPPING INDEX",
+                    Keyword::FULLTEXT
+                )
+                .as_str(),
                 self.parser.peek_token(),
             ),
         }
@@ -268,7 +291,12 @@ impl ParserContext<'_> {
             TokenWithLocation {
                 token: Token::Word(w),
                 ..
-            } if w.keyword == Keyword::FULLTEXT => self.parse_alter_column_fulltext(column_name),
+            } if w.keyword == Keyword::FULLTEXT => {
+                self.parser
+                    .expect_keyword(Keyword::INDEX)
+                    .context(error::SyntaxSnafu)?;
+                self.parse_alter_column_fulltext(column_name)
+            }
 
             TokenWithLocation {
                 token: Token::Word(w),
@@ -281,8 +309,18 @@ impl ParserContext<'_> {
                     options: SetIndexOperation::Inverted { column_name },
                 })
             }
+
+            TokenWithLocation {
+                token: Token::Word(w),
+                ..
+            } if w.value.eq_ignore_ascii_case("SKIPPING") => {
+                self.parser
+                    .expect_keyword(Keyword::INDEX)
+                    .context(error::SyntaxSnafu)?;
+                self.parse_alter_column_skipping(column_name)
+            }
             _ => self.expected(
-                format!("{:?} OR INVERTED INDEX", Keyword::FULLTEXT).as_str(),
+                format!("{:?} OR INVERTED OR SKIPPING INDEX", Keyword::FULLTEXT).as_str(),
                 self.parser.peek_token(),
             ),
         }
@@ -319,6 +357,35 @@ impl ParserContext<'_> {
             },
         })
     }
+
+    fn parse_alter_column_skipping(&mut self, column_name: Ident) -> Result<AlterTableOperation> {
+        let options = self
+            .parser
+            .parse_options(Keyword::WITH)
+            .context(error::SyntaxSnafu)?
+            .into_iter()
+            .map(parse_option_string)
+            .collect::<Result<HashMap<String, String>>>()?;
+
+        for key in options.keys() {
+            ensure!(
+                validate_column_skipping_index_create_option(key),
+                InvalidColumnOptionSnafu {
+                    name: column_name.to_string(),
+                    msg: format!("invalid SKIPPING INDEX option: {key}"),
+                }
+            );
+        }
+
+        Ok(AlterTableOperation::SetIndex {
+            options: SetIndexOperation::Skipping {
+                column_name,
+                options: options
+                    .try_into()
+                    .context(error::SetSkippingIndexOptionSnafu)?,
+            },
+        })
+    }
 }
 
 /// Parses a string literal and an optional string literal value.
@@ -891,7 +958,7 @@ mod tests {
 
     #[test]
     fn test_parse_alter_column_fulltext() {
-        let sql = "ALTER TABLE test_table MODIFY COLUMN a SET FULLTEXT WITH(analyzer='English',case_sensitive='false')";
+        let sql = "ALTER TABLE test_table MODIFY COLUMN a SET FULLTEXT INDEX WITH(analyzer='English',case_sensitive='false')";
         let mut result =
             ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default())
                 .unwrap();
@@ -928,7 +995,7 @@ mod tests {
             _ => unreachable!(),
         }
 
-        let sql = "ALTER TABLE test_table MODIFY COLUMN a UNSET FULLTEXT";
+        let sql = "ALTER TABLE test_table MODIFY COLUMN a UNSET FULLTEXT INDEX";
         let mut result =
             ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default())
                 .unwrap();
@@ -955,7 +1022,8 @@ mod tests {
             _ => unreachable!(),
         }
 
-        let invalid_sql = "ALTER TABLE test_table MODIFY COLUMN a SET FULLTEXT WITH('abcd'='true')";
+        let invalid_sql =
+            "ALTER TABLE test_table MODIFY COLUMN a SET FULLTEXT INDEX WITH('abcd'='true')";
         let result = ParserContext::create_with_dialect(
             invalid_sql,
             &GreptimeDbDialect {},
-Original file line number
+Diff line change
@@ Expand Up / @@ -380,6 +380,11 @@ impl ColumnSchema { @@
             );
             Ok(())
         }
+        pub fn unset_skipping_options(&mut self) -> Result<()> {
+            self.metadata.remove(SKIPPING_INDEX_KEY);
+            Ok(())
+        }
     }
     /// Column extended type set in column schema's metadata.
@@ Expand Down @@