diff --git a/src/sql/src/parsers/create_parser.rs b/src/sql/src/parsers/create_parser.rs index b154ee48abab..b84c965aa5f7 100644 --- 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 index 8bbf64d86e1b..e665ef257750 100644 --- 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 index 730694b8c67f..edb88136d9c8 100644 --- 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 index 5084b5b0155e..0c8aeaf09b99 100644 --- 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 ), ) .await; - assert!(matches!(result, Err(Error::TableOperation { .. }))); + assert_matches!(result, Err(Error::ParseSql { .. })); } #[apply(both_instances_cases)]