refactor: validate constraints eagerly (#3472)

* chore: validate constraints eagerly Signed-off-by: tison <[email protected]> * use timestamp column Signed-off-by: tison <[email protected]> * fixup Signed-off-by: tison <[email protected]> * lint Signed-off-by: tison <[email protected]> * compile Signed-off-by: tison <[email protected]> --------- Signed-off-by: tison <[email protected]>
GreptimeTeam · Mar 12, 2024 · cafb470 · cafb470
1 parent 7c895e2
commit cafb470
Show file tree

Hide file tree

Showing 4 changed files with 26 additions and 26 deletions.
diff --git a/src/sql/src/parsers/create_parser.rs b/src/sql/src/parsers/create_parser.rs
@@ -62,17 +62,21 @@ impl<'a> ParserContext<'a> {
         let _ = self.parser.next_token();
         self.parser
             .expect_keyword(Keyword::TABLE)
-            .context(error::SyntaxSnafu)?;
+            .context(SyntaxSnafu)?;
         let if_not_exists =
             self.parser
                 .parse_keywords(&[Keyword::IF, Keyword::NOT, Keyword::EXISTS]);
         let table_name = self.intern_parse_table_name()?;
         let (columns, constraints) = self.parse_columns()?;
+        if !columns.is_empty() {
+            validate_time_index(&columns, &constraints)?;
+        }
+
         let engine = self.parse_table_engine(common_catalog::consts::FILE_ENGINE)?;
         let options = self
             .parser
             .parse_options(Keyword::WITH)
-            .context(error::SyntaxSnafu)?
+            .context(SyntaxSnafu)?
             .into_iter()
             .filter_map(|option| {
                 if let Some(v) = parse_option_string(option.value) {
@@ -140,8 +144,12 @@ impl<'a> ParserContext<'a> {
         }
 
         let (columns, constraints) = self.parse_columns()?;
+        validate_time_index(&columns, &constraints)?;
 
         let partitions = self.parse_partitions()?;
+        if let Some(partitions) = &partitions {
+            validate_partitions(&columns, partitions)?;
+        }
 
         let engine = self.parse_table_engine(default_engine())?;
         let options = self
@@ -168,7 +176,6 @@ impl<'a> ParserContext<'a> {
             table_id: 0, // table id is assigned by catalog manager
             partitions,
         };
-        validate_create(&create_table)?;
 
         Ok(Statement::CreateTable(create_table))
     }
@@ -553,18 +560,8 @@ impl<'a> ParserContext<'a> {
     }
 }
 
-fn validate_create(create_table: &CreateTable) -> Result<()> {
-    if let Some(partitions) = &create_table.partitions {
-        validate_partitions(&create_table.columns, partitions)?;
-    }
-    validate_time_index(create_table)?;
-
-    Ok(())
-}
-
-fn validate_time_index(create_table: &CreateTable) -> Result<()> {
-    let time_index_constraints: Vec<_> = create_table
-        .constraints
+fn validate_time_index(columns: &[ColumnDef], constraints: &[TableConstraint]) -> Result<()> {
+    let time_index_constraints: Vec<_> = constraints
         .iter()
         .filter_map(|c| {
             if let TableConstraint::Unique {
@@ -605,8 +602,7 @@ fn validate_time_index(create_table: &CreateTable) -> Result<()> {
     // It's safe to use time_index_constraints[0][0],
     // we already check the bound above.
     let time_index_column_ident = &time_index_constraints[0][0];
-    let time_index_column = create_table
-        .columns
+    let time_index_column = columns
         .iter()
         .find(|c| c.name.value == *time_index_column_ident.value)
         .with_context(|| InvalidTimeIndexSnafu {
@@ -753,7 +749,7 @@ mod tests {
     fn test_validate_external_table_options() {
         let sql = "CREATE EXTERNAL TABLE city (
             host string,
-            ts int64,
+            ts timestamp,
             cpu float64 default 0,
             memory float64,
             TIME INDEX (ts),
@@ -836,7 +832,7 @@ mod tests {
     fn test_parse_create_external_table_with_schema() {
         let sql = "CREATE EXTERNAL TABLE city (
             host string,
-            ts int64,
+            ts timestamp,
             cpu float32 default 0,
             memory float64,
             TIME INDEX (ts),
@@ -859,7 +855,7 @@ mod tests {
 
                 let columns = &c.columns;
                 assert_column_def(&columns[0], "host", "STRING");
-                assert_column_def(&columns[1], "ts", "BIGINT");
+                assert_column_def(&columns[1], "ts", "TIMESTAMP");
                 assert_column_def(&columns[2], "cpu", "FLOAT");
                 assert_column_def(&columns[3], "memory", "DOUBLE");
 
@@ -938,7 +934,7 @@ ENGINE=mito";
         let _ = result.unwrap();
 
         let sql = r"
-CREATE TABLE rcx ( a INT, b STRING, c INT )
+CREATE TABLE rcx ( ts TIMESTAMP TIME INDEX, a INT, b STRING, c INT )
 PARTITION ON COLUMNS(x) ()
 ENGINE=mito";
         let result =
@@ -1326,7 +1322,7 @@ ENGINE=mito";
     #[test]
     fn test_parse_partitions_with_error_syntax() {
         let sql = r"
-CREATE TABLE rcx ( a INT, b STRING, c INT )
+CREATE TABLE rcx ( ts TIMESTAMP TIME INDEX, a INT, b STRING, c INT )
 PARTITION COLUMNS(c, a) (
     a < 10,
     a > 10 AND a < 20,
@@ -1355,7 +1351,7 @@ ENGINE=mito";
     #[test]
     fn test_parse_partitions_unreferenced_column() {
         let sql = r"
-CREATE TABLE rcx ( a INT, b STRING, c INT )
+CREATE TABLE rcx ( ts TIMESTAMP TIME INDEX, a INT, b STRING, c INT )
 PARTITION ON COLUMNS(c, a) (
     b = 'foo'
 )
@@ -1371,7 +1367,7 @@ ENGINE=mito";
     #[test]
     fn test_parse_partitions_not_binary_expr() {
         let sql = r"
-CREATE TABLE rcx ( a INT, b STRING, c INT )
+CREATE TABLE rcx ( ts TIMESTAMP TIME INDEX, a INT, b STRING, c INT )
 PARTITION ON COLUMNS(c, a) (
     b
 )

diff --git a/src/sql/src/statements/create.rs b/src/sql/src/statements/create.rs
@@ -229,6 +229,8 @@ pub struct CreateTableLike {
 
 #[cfg(test)]
 mod tests {
+    use std::assert_matches::assert_matches;
+
     use crate::dialect::GreptimeDbDialect;
     use crate::error::Error;
     use crate::parser::{ParseOptions, ParserContext};
@@ -378,6 +380,6 @@ ENGINE=mito
 ";
         let result =
             ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default());
-        assert!(matches!(result, Err(Error::InvalidTableOption { .. })));
+        assert_matches!(result, Err(Error::InvalidTableOption { .. }))
     }
 }
diff --git a/tests-integration/src/lib.rs b/tests-integration/src/lib.rs
@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#![feature(assert_matches)]
 pub mod cluster;
 mod grpc;
 mod influxdb;

diff --git a/tests-integration/src/tests/instance_test.rs b/tests-integration/src/tests/instance_test.rs
@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use std::assert_matches::assert_matches;
 use std::env;
 use std::sync::Arc;
 
@@ -639,7 +640,7 @@ async fn test_execute_external_create_without_ts(instance: Arc<dyn MockInstance>
         ),
     )
     .await;
-    assert!(matches!(result, Err(Error::TableOperation { .. })));
+    assert_matches!(result, Err(Error::ParseSql { .. }));
 }
 
 #[apply(both_instances_cases)]