refactor!: unify sql options into OptionMap (#3792)

* unify sql options into OptionMap Signed-off-by: tison <[email protected]> * fixup Signed-off-by: tison <[email protected]> * Update src/sql/src/util.rs * drop legacy regions option Signed-off-by: tison <[email protected]> * fixup Signed-off-by: tison <[email protected]> * fixup Signed-off-by: tison <[email protected]> --------- Signed-off-by: tison <[email protected]>
GreptimeTeam · Apr 25, 2024 · bba3108 · bba3108
1 parent 9524ec8
commit bba3108
Show file tree

Hide file tree

Showing 23 changed files with 110 additions and 199 deletions.
diff --git a/docs/schema-structs.md b/docs/schema-structs.md
@@ -73,7 +73,7 @@ CREATE TABLE cpu (
     usage_system DOUBLE,
     datacenter STRING,
     TIME INDEX (ts),
-    PRIMARY KEY(datacenter, host)) ENGINE=mito WITH(regions=1);
+    PRIMARY KEY(datacenter, host)) ENGINE=mito;
 ```
 
 Then the table's `TableMeta` may look like this:
@@ -249,7 +249,7 @@ CREATE TABLE cpu (
     usage_system DOUBLE,
     datacenter STRING,
     TIME INDEX (ts),
-    PRIMARY KEY(datacenter, host)) ENGINE=mito WITH(regions=1);
+    PRIMARY KEY(datacenter, host)) ENGINE=mito;
 
 select ts, usage_system from cpu;
 ```

diff --git a/src/cmd/src/cli/export.rs b/src/cmd/src/cli/export.rs
@@ -492,9 +492,7 @@ mod tests {
 )
 
 ENGINE=mito
-WITH(
-  regions = 1
-);
+;
 "#;
         assert_eq!(res.trim(), expect.trim());
 

diff --git a/src/frontend/src/instance.rs b/src/frontend/src/instance.rs
@@ -650,7 +650,7 @@ mod tests {
                             ts TIMESTAMP,
                             TIME INDEX (ts),
                             PRIMARY KEY(host)
-                        ) engine=mito with(regions=1);"#;
+                        ) engine=mito;"#;
         replace_test(sql, plugins.clone(), &query_ctx);
 
         // test drop table

diff --git a/src/operator/src/expr_factory.rs b/src/operator/src/expr_factory.rs
@@ -36,7 +36,6 @@ use sql::ast::{ColumnDef, ColumnOption, TableConstraint};
 use sql::statements::alter::{AlterTable, AlterTableOperation};
 use sql::statements::create::{CreateExternalTable, CreateTable, TIME_INDEX};
 use sql::statements::{column_def_to_schema, sql_column_def_to_grpc_column_def};
-use sql::util::to_lowercase_options_map;
 use table::requests::{TableOptions, FILE_TABLE_META_KEY};
 use table::table_reference::TableReference;
 
@@ -190,8 +189,7 @@ pub fn create_to_expr(create: &CreateTable, query_ctx: QueryContextRef) -> Resul
 
     let time_index = find_time_index(&create.constraints)?;
     let table_options = HashMap::from(
-        &TableOptions::try_from(&to_lowercase_options_map(&create.options))
-            .context(UnrecognizedTableOptionSnafu)?,
+        &TableOptions::try_from(create.options.as_ref()).context(UnrecognizedTableOptionSnafu)?,
     );
 
     let primary_keys = find_primary_keys(&create.columns, &create.constraints)?;
@@ -502,7 +500,7 @@ mod tests {
 
     #[test]
     fn test_create_to_expr() {
-        let sql = "CREATE TABLE monitor (host STRING,ts TIMESTAMP,TIME INDEX (ts),PRIMARY KEY(host)) ENGINE=mito WITH(regions=1, ttl='3days', write_buffer_size='1024KB');";
+        let sql = "CREATE TABLE monitor (host STRING,ts TIMESTAMP,TIME INDEX (ts),PRIMARY KEY(host)) ENGINE=mito WITH(ttl='3days', write_buffer_size='1024KB');";
         let stmt =
             ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default())
                 .unwrap()

diff --git a/src/query/src/sql/show_create_table.rs b/src/query/src/sql/show_create_table.rs
@@ -14,75 +14,46 @@
 
 //! Implementation of `SHOW CREATE TABLE` statement.
 
-use std::fmt::Display;
+use std::collections::HashMap;
 
 use datatypes::schema::{ColumnDefaultConstraint, ColumnSchema, SchemaRef, COMMENT_KEY};
 use humantime::format_duration;
 use snafu::ResultExt;
 use sql::ast::{
-    ColumnDef, ColumnOption, ColumnOptionDef, Expr, Ident, ObjectName, SqlOption, TableConstraint,
-    Value as SqlValue,
+    ColumnDef, ColumnOption, ColumnOptionDef, Expr, Ident, ObjectName, TableConstraint,
 };
 use sql::dialect::GreptimeDbDialect;
 use sql::parser::ParserContext;
 use sql::statements::create::{CreateTable, TIME_INDEX};
-use sql::statements::{self};
+use sql::statements::{self, OptionMap};
 use table::metadata::{TableInfoRef, TableMeta};
-use table::requests::FILE_TABLE_META_KEY;
+use table::requests::{FILE_TABLE_META_KEY, TTL_KEY, WRITE_BUFFER_SIZE_KEY};
 
 use crate::error::{ConvertSqlTypeSnafu, ConvertSqlValueSnafu, Result, SqlSnafu};
 
-#[inline]
-fn number_value<T: Display>(n: T) -> SqlValue {
-    SqlValue::Number(format!("{}", n), false)
-}
-
-#[inline]
-fn string_value(s: impl Into<String>) -> SqlValue {
-    SqlValue::SingleQuotedString(s.into())
-}
-
-#[inline]
-fn sql_option(name: &str, value: SqlValue) -> SqlOption {
-    SqlOption {
-        name: name.into(),
-        value: Expr::Value(value),
-    }
-}
-
-fn create_sql_options(table_meta: &TableMeta) -> Vec<SqlOption> {
+fn create_sql_options(table_meta: &TableMeta) -> OptionMap {
     let table_opts = &table_meta.options;
-    let mut options = Vec::with_capacity(4 + table_opts.extra_options.len());
-
-    if !table_meta.region_numbers.is_empty() {
-        options.push(sql_option(
-            "regions",
-            number_value(table_meta.region_numbers.len()),
-        ));
-    }
+    let mut options = HashMap::with_capacity(4 + table_opts.extra_options.len());
 
     if let Some(write_buffer_size) = table_opts.write_buffer_size {
-        options.push(sql_option(
-            "write_buffer_size",
-            string_value(write_buffer_size.to_string()),
-        ));
+        options.insert(
+            WRITE_BUFFER_SIZE_KEY.to_string(),
+            write_buffer_size.to_string(),
+        );
     }
     if let Some(ttl) = table_opts.ttl {
-        options.push(sql_option(
-            "ttl",
-            string_value(format_duration(ttl).to_string()),
-        ));
+        options.insert(TTL_KEY.to_string(), format_duration(ttl).to_string());
     }
 
     for (k, v) in table_opts
         .extra_options
         .iter()
         .filter(|(k, _)| k != &FILE_TABLE_META_KEY)
     {
-        options.push(sql_option(k, string_value(v)));
+        options.insert(k.to_string(), v.to_string());
     }
 
-    options
+    OptionMap { map: options }
 }
 
 #[inline]
@@ -265,9 +236,7 @@ CREATE TABLE IF NOT EXISTS "system_metrics" (
   PRIMARY KEY ("id", "host")
 )
 ENGINE=mito
-WITH(
-  regions = 3
-)"#,
+"#,
             sql
         );
     }

diff --git a/src/sql/src/error.rs b/src/sql/src/error.rs
@@ -21,6 +21,7 @@ use common_time::timestamp::TimeUnit;
 use common_time::Timestamp;
 use datatypes::prelude::{ConcreteDataType, Value};
 use snafu::{Location, Snafu};
+use sqlparser::ast::Ident;
 use sqlparser::parser::ParserError;
 
 use crate::ast::{Expr, Value as SqlValue};
@@ -141,6 +142,13 @@ pub enum Error {
     #[snafu(display("Unrecognized table option key: {}", key))]
     InvalidTableOption { key: String, location: Location },
 
+    #[snafu(display("Unrecognized table option key: {}, value: {}", key, value))]
+    InvalidTableOptionValue {
+        key: Ident,
+        value: Expr,
+        location: Location,
+    },
+
     #[snafu(display("Failed to serialize column default constraint"))]
     SerializeColumnDefaultConstraint {
         location: Location,
@@ -201,6 +209,7 @@ impl ErrorExt for Error {
             | InvalidDefault { .. } => StatusCode::InvalidSyntax,
 
             InvalidColumnOption { .. }
+            | InvalidTableOptionValue { .. }
             | InvalidDatabaseName { .. }
             | ColumnTypeMismatch { .. }
             | InvalidTableName { .. }

diff --git a/src/sql/src/parsers/copy_parser.rs b/src/sql/src/parsers/copy_parser.rs
@@ -129,10 +129,8 @@ impl<'a> ParserContext<'a> {
 
         let with = options
             .into_iter()
-            .filter_map(|option| {
-                parse_option_string(option.value).map(|v| (option.name.value.to_lowercase(), v))
-            })
-            .collect();
+            .map(parse_option_string)
+            .collect::<Result<With>>()?;
 
         let connection_options = self
             .parser
@@ -141,10 +139,8 @@ impl<'a> ParserContext<'a> {
 
         let connection = connection_options
             .into_iter()
-            .filter_map(|option| {
-                parse_option_string(option.value).map(|v| (option.name.value.to_lowercase(), v))
-            })
-            .collect();
+            .map(parse_option_string)
+            .collect::<Result<Connection>>()?;
 
         Ok((with, connection, location))
     }

diff --git a/src/sql/src/parsers/create_parser.rs b/src/sql/src/parsers/create_parser.rs
@@ -34,8 +34,8 @@ use crate::parser::ParserContext;
 use crate::statements::create::{
     CreateDatabase, CreateExternalTable, CreateTable, CreateTableLike, Partitions, TIME_INDEX,
 };
-use crate::statements::get_data_type_by_alias_name;
 use crate::statements::statement::Statement;
+use crate::statements::{get_data_type_by_alias_name, OptionMap};
 use crate::util::parse_option_string;
 
 pub const ENGINE: &str = "ENGINE";
@@ -73,32 +73,12 @@ impl<'a> ParserContext<'a> {
         }
 
         let engine = self.parse_table_engine(common_catalog::consts::FILE_ENGINE)?;
-        let options = self
-            .parser
-            .parse_options(Keyword::WITH)
-            .context(SyntaxSnafu)?
-            .into_iter()
-            .filter_map(|option| {
-                if let Some(v) = parse_option_string(option.value) {
-                    Some((option.name.value.to_lowercase(), v))
-                } else {
-                    None
-                }
-            })
-            .collect::<HashMap<String, String>>();
-        for key in options.keys() {
-            ensure!(
-                validate_table_option(key),
-                InvalidTableOptionSnafu {
-                    key: key.to_string()
-                }
-            );
-        }
+        let options = self.parse_create_table_options()?;
         Ok(Statement::CreateExternalTable(CreateExternalTable {
             name: table_name,
             columns,
             constraints,
-            options: options.into(),
+            options,
             if_not_exists,
             engine,
         }))
@@ -149,20 +129,7 @@ impl<'a> ParserContext<'a> {
         }
 
         let engine = self.parse_table_engine(default_engine())?;
-        let options = self
-            .parser
-            .parse_options(Keyword::WITH)
-            .context(error::SyntaxSnafu)?;
-        for option in options.iter() {
-            ensure!(
-                validate_table_option(&option.name.value),
-                InvalidTableOptionSnafu {
-                    key: option.name.value.to_string()
-                }
-            );
-        }
-        // Sorts options so that `test_display_create_table` can always pass.
-        let options = options.into_iter().sorted().collect();
+        let options = self.parse_create_table_options()?;
         let create_table = CreateTable {
             if_not_exists,
             name: table_name,
@@ -177,6 +144,25 @@ impl<'a> ParserContext<'a> {
         Ok(Statement::CreateTable(create_table))
     }
 
+    fn parse_create_table_options(&mut self) -> Result<OptionMap> {
+        let options = self
+            .parser
+            .parse_options(Keyword::WITH)
+            .context(SyntaxSnafu)?
+            .into_iter()
+            .map(parse_option_string)
+            .collect::<Result<HashMap<String, String>>>()?;
+        for key in options.keys() {
+            ensure!(
+                validate_table_option(key),
+                InvalidTableOptionSnafu {
+                    key: key.to_string()
+                }
+            );
+        }
+        Ok(options.into())
+    }
+
     /// "PARTITION BY ..." syntax:
     // TODO(ruihang): docs
     fn parse_partitions(&mut self) -> Result<Option<Partitions>> {
@@ -1415,7 +1401,7 @@ ENGINE=mito";
                              memory float64,
                              TIME INDEX (ts),
                              PRIMARY KEY(ts, host)) engine=mito
-                             with(regions=1);
+                             with(ttl='10s');
          ";
         let result =
             ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default())
@@ -1449,9 +1435,9 @@ ENGINE=mito";
                     }
                 );
                 let options = &c.options;
-                assert_eq!(1, options.len());
-                assert_eq!("regions", &options[0].name.to_string());
-                assert_eq!("1", &options[0].value.to_string());
+                assert_eq!(1, options.map.len());
+                let (k, v) = options.map.iter().next().unwrap();
+                assert_eq!(("ttl", "10s"), (k.as_str(), v.as_str()));
             }
             _ => unreachable!(),
         }
@@ -1465,8 +1451,7 @@ ENGINE=mito";
                              cpu float64 default 0,
                              memory float64,
                              TIME INDEX (ts, host),
-                             PRIMARY KEY(ts, host)) engine=mito
-                             with(regions=1);
+                             PRIMARY KEY(ts, host)) engine=mito;
          ";
         let result =
             ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default());
@@ -1483,8 +1468,7 @@ ENGINE=mito";
                              cpu float64 default 0,
                              memory float64,
                              TIME INDEX (ts, host),
-                             PRIMARY KEY(ts, host)) engine=mito
-                             with(regions=1);
+                             PRIMARY KEY(ts, host)) engine=mito;
          ";
         let result =
             ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default());
@@ -1498,8 +1482,7 @@ ENGINE=mito";
                              t timestamp,
                              memory float64,
                              TIME INDEX (t),
-                             PRIMARY KEY(ts, host)) engine=mito
-                             with(regions=1);
+                             PRIMARY KEY(ts, host)) engine=mito;
          ";
         let result =
             ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default());
-Original file line number
+Diff line change
@@ Expand Up / @@ -492,9 +492,7 @@ mod tests { @@
     )
     ENGINE=mito
-    WITH(
-      regions = 1
-    );
+    ;
     "#;
             assert_eq!(res.trim(), expect.trim());
@@ Expand Down @@