From daabfae58e8ecdcfaab25473555efc9b851390ac Mon Sep 17 00:00:00 2001 From: Dennis Liu Date: Tue, 17 Oct 2023 16:18:10 +0800 Subject: [PATCH 1/4] feat(query_frontend): infer timestamp for table creation This commit addresses issue #1144. It aims to automatically infer the timestamp column during table creation when possible. Previously, users had to explicitly set a timestamp constraint, adding unnecessary complexity. Now, the system will detect if there is only one timestamp column and set it as the constraint. - Added `build_and_check_timestamp_key_constraint` to check and infer. - Modified `create_table` to call the new function. --- query_frontend/src/parser.rs | 43 +++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/query_frontend/src/parser.rs b/query_frontend/src/parser.rs index 20fd585d37..3fa56714bd 100644 --- a/query_frontend/src/parser.rs +++ b/query_frontend/src/parser.rs @@ -418,7 +418,7 @@ impl<'a> Parser<'a> { } } - build_timestamp_key_constraint(&columns, &mut constraints); + build_and_check_timestamp_key_constraint(&columns, &mut constraints); Ok((columns, constraints)) } @@ -791,6 +791,47 @@ fn check_column_expr_validity_in_hash(column: &Ident, columns: &[ColumnDef]) -> valid_column.is_some() } +/// Builds and checks for the existence of a timestamp key constraint named +/// __ts_key within the given list of table constraints. If such a constraint +/// does not exist, the function searches for a unique timestamp column and +/// creates a new constraint for it. +fn build_and_check_timestamp_key_constraint( + col_defs: &[ColumnDef], + constraints: &mut Vec, +) { + // try to create timestamp key constraint from ColumnOption::DialectSpecific + // tokens + build_timestamp_key_constraint(col_defs, constraints); + + // Now check if there's a __ts_key constraint + let no_timestamp_constraint = constraints + .iter() + .all(|constraint| !is_timestamp_key_constraint(constraint)); + + if no_timestamp_constraint { + // If a timestamp constraint doesn't exist, start looking for a unique timestamp + // column + let mut ts_column_iterator = col_defs + .iter() + .filter(|col_def| matches!(col_def.data_type, DataType::Timestamp(_, _))); + + if let Some(ts_column) = ts_column_iterator.next() { + if ts_column_iterator.next().is_none() { + // Create a timestamp constraint if the column is unique + let constraint = TableConstraint::Unique { + name: Some(Ident { + value: TS_KEY.to_owned(), + quote_style: None, + }), + columns: vec![ts_column.name.clone()], + is_primary: false, + }; + constraints.push(constraint); + } + } + } +} + // Build the tskey constraint from the column definitions if any. fn build_timestamp_key_constraint(col_defs: &[ColumnDef], constraints: &mut Vec) { for col_def in col_defs { From e58004bcd4e2eafdaba686dfbec9e8a1f21a9da7 Mon Sep 17 00:00:00 2001 From: Dennis Liu Date: Tue, 17 Oct 2023 18:05:52 +0800 Subject: [PATCH 2/4] test(query_frontend): update test expectations for `create_table` - Refactor test cases for `create_table` in `parser.rs` to align with the latest timestamp inference changes made in the previous commit. - Update the expected statements to include unique constraints for timestamp columns, as influenced by the modifications in timestamp inference. --- query_frontend/src/parser.rs | 42 +++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/query_frontend/src/parser.rs b/query_frontend/src/parser.rs index 3fa56714bd..90095e1ef4 100644 --- a/query_frontend/src/parser.rs +++ b/query_frontend/src/parser.rs @@ -995,34 +995,52 @@ mod tests { expect_parse_ok(sql, expected).unwrap(); // positive case, multiple columns + let columns = vec![ + make_column_def("c1", DataType::Timestamp(None, TimezoneInfo::None)), + make_column_def("c2", DataType::Double), + make_column_def("c3", DataType::String), + ]; + let sql = "CREATE TABLE mytbl(c1 timestamp, c2 double, c3 string,) ENGINE = XX"; let expected = Statement::Create(Box::new(CreateTable { if_not_exists: false, table_name: make_table_name("mytbl"), - columns: vec![ - make_column_def("c1", DataType::Timestamp(None, TimezoneInfo::None)), - make_column_def("c2", DataType::Double), - make_column_def("c3", DataType::String), - ], + columns: columns.clone(), engine: "XX".to_string(), - constraints: vec![], + constraints: vec![TableConstraint::Unique { + name: Some(Ident { + value: TS_KEY.to_owned(), + quote_style: None, + }), + columns: vec![columns[0].name.clone()], + is_primary: false, + }], options: vec![], partition: None, })); expect_parse_ok(sql, expected).unwrap(); // positive case, multiple columns with comment + let columns = vec![ + make_column_def("c1", DataType::Timestamp(None, TimezoneInfo::None)), + make_comment_column_def("c2", DataType::Double, "id".to_string()), + make_comment_column_def("c3", DataType::String, "name".to_string()), + ]; + let sql = "CREATE TABLE mytbl(c1 timestamp, c2 double comment 'id', c3 string comment 'name',) ENGINE = XX"; let expected = Statement::Create(Box::new(CreateTable { if_not_exists: false, table_name: make_table_name("mytbl"), - columns: vec![ - make_column_def("c1", DataType::Timestamp(None, TimezoneInfo::None)), - make_comment_column_def("c2", DataType::Double, "id".to_string()), - make_comment_column_def("c3", DataType::String, "name".to_string()), - ], + columns: columns.clone(), engine: "XX".to_string(), - constraints: vec![], + constraints: vec![TableConstraint::Unique { + name: Some(Ident { + value: TS_KEY.to_owned(), + quote_style: None, + }), + columns: vec![columns[0].name.clone()], + is_primary: false, + }], options: vec![], partition: None, })); From c59aed1a612c608df8e7bcec5b95c06980f69079 Mon Sep 17 00:00:00 2001 From: Dennis Liu Date: Tue, 17 Oct 2023 23:22:08 +0800 Subject: [PATCH 3/4] refactor: rename the timestamp constraint inference function - Rename from `build_and_check_timestamp_key_constraint` to `build_or_infer_timestamp_key_constraint` - Modify the function's comment --- query_frontend/src/parser.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/query_frontend/src/parser.rs b/query_frontend/src/parser.rs index 90095e1ef4..33234b3839 100644 --- a/query_frontend/src/parser.rs +++ b/query_frontend/src/parser.rs @@ -418,7 +418,7 @@ impl<'a> Parser<'a> { } } - build_and_check_timestamp_key_constraint(&columns, &mut constraints); + build_or_infer_timestamp_key_constraint(&columns, &mut constraints); Ok((columns, constraints)) } @@ -791,11 +791,11 @@ fn check_column_expr_validity_in_hash(column: &Ident, columns: &[ColumnDef]) -> valid_column.is_some() } -/// Builds and checks for the existence of a timestamp key constraint named -/// __ts_key within the given list of table constraints. If such a constraint -/// does not exist, the function searches for a unique timestamp column and -/// creates a new constraint for it. -fn build_and_check_timestamp_key_constraint( +/// Builds for the existence of a timestamp key constraint named +/// __ts_key within the given list of table constraints first. If such a +/// constraint does not exist, the function will try to search for a unique +/// timestamp column and create a new constraint for it. +fn build_or_infer_timestamp_key_constraint( col_defs: &[ColumnDef], constraints: &mut Vec, ) { From 1c1706bf0807d807495b16a7531d9ddc0eafcfbb Mon Sep 17 00:00:00 2001 From: Dennis Liu Date: Tue, 17 Oct 2023 23:58:35 +0800 Subject: [PATCH 4/4] test: add explicit timestamp key case - Added a test case that includes two timestamp columns and explicitly selects the c2 field as the timestamp key. The test passes locally. --- query_frontend/src/parser.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/query_frontend/src/parser.rs b/query_frontend/src/parser.rs index 33234b3839..86f01d4b16 100644 --- a/query_frontend/src/parser.rs +++ b/query_frontend/src/parser.rs @@ -1046,6 +1046,34 @@ mod tests { })); expect_parse_ok(sql, expected).unwrap(); + // Explicitly declare `c2` as timestamp key column + let columns = vec![ + make_column_def("c1", DataType::Timestamp(None, TimezoneInfo::None)), + make_column_def("c2", DataType::Timestamp(None, TimezoneInfo::None)), + make_column_def("c3", DataType::String), + make_column_def("c4", DataType::Double), + ]; + + let sql = + "CREATE TABLE mytbl(c1 timestamp, c2 timestamp, c3 string, c4 double, timestamp key(c2),) ENGINE = XX"; + let expected = Statement::Create(Box::new(CreateTable { + if_not_exists: false, + table_name: make_table_name("mytbl"), + columns: columns.clone(), + engine: "XX".to_string(), + constraints: vec![TableConstraint::Unique { + name: Some(Ident { + value: TS_KEY.to_owned(), + quote_style: None, + }), + columns: vec![columns[1].name.clone()], + is_primary: false, + }], + options: vec![], + partition: None, + })); + expect_parse_ok(sql, expected).unwrap(); + // Error cases: Invalid sql let sql = "CREATE TABLE t(c1 timestamp) AS"; expect_parse_error(