fix: parsing time index column option

src/sql/src/error.rs

@@ -73,6 +73,13 @@ pub enum Error {
     #[snafu(display("Invalid SQL, error: {}", msg))]
     InvalidSql { msg: String, backtrace: Backtrace },
 
+    #[snafu(display("Invalid column option, column name: {}, error: {}", name, msg))]
+    InvalidColumnOption {
+        name: String,
+        msg: String,
+        backtrace: Backtrace,
+    },
+
     #[snafu(display("SQL data type not supported yet: {:?}", t))]
     SqlTypeNotSupported {
         t: crate::ast::DataType,
@@ -141,9 +148,10 @@ impl ErrorExt for Error {
             | SqlTypeNotSupported { .. }
             | InvalidDefault { .. } => StatusCode::InvalidSyntax,
 
-            InvalidDatabaseName { .. } | ColumnTypeMismatch { .. } | InvalidTableName { .. } => {
-                StatusCode::InvalidArguments
-            }
+            InvalidColumnOption { .. }
+            | InvalidDatabaseName { .. }
+            | ColumnTypeMismatch { .. }
+            | InvalidTableName { .. } => StatusCode::InvalidArguments,
             UnsupportedAlterTableStatement { .. } => StatusCode::InvalidSyntax,
             SerializeColumnDefaultConstraint { source, .. } => source.status_code(),
             ConvertToGrpcDataType { source, .. } => source.status_code(),

src/sql/src/parsers/create_parser.rs

@@ -18,14 +18,14 @@ use itertools::Itertools;
 use mito::engine;
 use once_cell::sync::Lazy;
 use snafu::{ensure, OptionExt, ResultExt};
-use sqlparser::ast::ColumnOption::NotNull;
-use sqlparser::ast::{ColumnOptionDef, DataType, Value};
+use sqlparser::ast::{ColumnOption, ColumnOptionDef, DataType, Value};
 use sqlparser::dialect::keywords::Keyword;
 use sqlparser::parser::IsOptional::Mandatory;
+use sqlparser::parser::{Parser, ParserError};
 use sqlparser::tokenizer::{Token, Word};
 
 use crate::ast::{ColumnDef, Ident, TableConstraint, Value as SqlValue};
-use crate::error::{self, InvalidTimeIndexSnafu, Result, SyntaxSnafu};
+use crate::error::{self, InvalidColumnOptionSnafu, InvalidTimeIndexSnafu, Result, SyntaxSnafu};
 use crate::parser::ParserContext;
 use crate::statements::create::{
     CreateDatabase, CreateTable, PartitionEntry, Partitions, TIME_INDEX,
@@ -253,74 +253,160 @@ impl<'a> ParserContext<'a> {
         columns: &mut Vec<ColumnDef>,
         constraints: &mut Vec<TableConstraint>,
     ) -> Result<()> {
-        let column = self
-            .parser
+        let mut column = self
             .parse_column_def()
             .context(SyntaxSnafu { sql: self.sql })?;
 
-        if !matches!(
-            column.data_type,
-            DataType::Timestamp(_, _) | DataType::BigInt(_)
-        ) || matches!(self.parser.peek_token(), Token::Comma)
-        {
-            columns.push(column);
-            return Ok(());
-        }
+        let mut time_index_opt_idx = None;
+        for (index, opt) in column.options.iter().enumerate() {
+            if let ColumnOption::DialectSpecific(tokens) = &opt.option {
+                if matches!(
+                    &tokens[..],
+                    [
+                        Token::Word(Word {
+                            keyword: Keyword::TIME,
+                            ..
+                        }),
+                        Token::Word(Word {
+                            keyword: Keyword::INDEX,
+                            ..
+                        })
+                    ]
+                ) {
+                    let constraint = TableConstraint::Unique {
+                        name: Some(Ident {
+                            value: TIME_INDEX.to_owned(),
+                            quote_style: None,
+                        }),
+                        columns: vec![Ident {
+                            value: column.name.value.clone(),
+                            quote_style: None,
+                        }],
+                        is_primary: false,
+                    };
 
-        // for supporting `ts TIMESTAMP TIME INDEX,` syntax.
-        self.parse_time_index(column, columns, constraints)
-    }
+                    constraints.push(constraint);
+                    time_index_opt_idx = Some(index);
+                }
+            }
+        }
 
-    fn parse_time_index(
-        &mut self,
-        mut column: ColumnDef,
-        columns: &mut Vec<ColumnDef>,
-        constraints: &mut Vec<TableConstraint>,
-    ) -> Result<()> {
-        self.parser
-            .expect_keywords(&[Keyword::TIME, Keyword::INDEX])
-            .context(error::UnexpectedSnafu {
-                sql: self.sql,
-                expected: "TIME INDEX",
-                actual: self.peek_token_as_string(),
-            })?;
+        if let Some(index) = time_index_opt_idx {
+            ensure!(
+                !column.options.contains(&ColumnOptionDef {
+                    option: ColumnOption::Null,
+                    name: None,
+                }),
+                InvalidColumnOptionSnafu {
+                    name: column.name.to_string(),
+                    msg: "time index column can't be null",
+                }
+            );
+            ensure!(
+                matches!(
+                    column.data_type,
+                    DataType::Timestamp(_, _) | DataType::BigInt(_)
+                ),
+                InvalidColumnOptionSnafu {
+                    name: column.name.to_string(),
+                    msg: "time index column data type should be timestamp or bigint",
+                }
+            );
 
-        let constraint = TableConstraint::Unique {
-            name: Some(Ident {
-                value: TIME_INDEX.to_owned(),
-                quote_style: None,
-            }),
-            columns: vec![Ident {
-                value: column.name.value.clone(),
-                quote_style: None,
-            }],
-            is_primary: false,
-        };
+            let not_null_opt = ColumnOptionDef {
+                option: ColumnOption::NotNull,
+                name: None,
+            };
 
-        // TIME INDEX option means NOT NULL implicitly.
-        column.options = vec![ColumnOptionDef {
-            name: None,
-            option: NotNull,
-        }];
-        columns.push(column);
-        constraints.push(constraint);
+            if !column.options.contains(&not_null_opt) {
+                column.options.push(not_null_opt);
+            }
 
-        if matches!(self.parser.peek_token(), Token::Comma | Token::RParen) {
-            return Ok(());
+            column.options.remove(index);
         }
 
-        self.parser
-            .expect_keywords(&[Keyword::NOT, Keyword::NULL])
-            .context(error::UnexpectedSnafu {
-                sql: self.sql,
-                expected: "NOT NULL",
-                actual: self.peek_token_as_string(),
-            })?;
+        columns.push(column);
 
         Ok(())
     }
 
-    // Copy from sqlparser by boyan
+    pub fn parse_column_def(&mut self) -> std::result::Result<ColumnDef, ParserError> {
+        let parser = &mut self.parser;
+
+        let name = parser.parse_identifier()?;
+        let data_type = parser.parse_data_type()?;
+        let collation = if parser.parse_keyword(Keyword::COLLATE) {
+            Some(parser.parse_object_name()?)
+        } else {
+            None
+        };
+        let mut options = vec![];
+        loop {
+            if parser.parse_keyword(Keyword::CONSTRAINT) {
+                let name = Some(parser.parse_identifier()?);
+                if let Some(option) = Self::parse_optional_column_option(parser)? {
+                    options.push(ColumnOptionDef { name, option });
+                } else {
+                    return parser.expected(
+                        "constraint details after CONSTRAINT <name>",
+                        parser.peek_token(),
+                    );
+                }
+            } else if let Some(option) = Self::parse_optional_column_option(parser)? {
+                options.push(ColumnOptionDef { name: None, option });
+            } else {
+                break;
+            };
+        }
+        Ok(ColumnDef {
+            name,
+            data_type,
+            collation,
+            options,
+        })
+    }
+
+    fn parse_optional_column_option(
+        parser: &mut Parser<'a>,
+    ) -> std::result::Result<Option<ColumnOption>, ParserError> {
+        if parser.parse_keywords(&[Keyword::CHARACTER, Keyword::SET]) {
+            Ok(Some(ColumnOption::CharacterSet(
+                parser.parse_object_name()?,
+            )))
+        } else if parser.parse_keywords(&[Keyword::NOT, Keyword::NULL]) {
+            Ok(Some(ColumnOption::NotNull))
+        } else if parser.parse_keywords(&[Keyword::COMMENT]) {
+            match parser.next_token() {
+                Token::SingleQuotedString(value, ..) => Ok(Some(ColumnOption::Comment(value))),
+                unexpected => parser.expected("string", unexpected),
+            }
+        } else if parser.parse_keyword(Keyword::NULL) {
+            Ok(Some(ColumnOption::Null))
+        } else if parser.parse_keyword(Keyword::DEFAULT) {
+            Ok(Some(ColumnOption::Default(parser.parse_expr()?)))
+        } else if parser.parse_keywords(&[Keyword::PRIMARY, Keyword::KEY]) {
+            Ok(Some(ColumnOption::Unique { is_primary: true }))
+        } else if parser.parse_keyword(Keyword::UNIQUE) {
+            Ok(Some(ColumnOption::Unique { is_primary: false }))
+        } else if parser.parse_keywords(&[Keyword::TIME, Keyword::INDEX]) {
+            // Use a DialectSpecific option for time index
+            Ok(Some(ColumnOption::DialectSpecific(vec![
+                Token::Word(Word {
+                    value: "TIME".to_string(),
+                    quote_style: None,
+                    keyword: Keyword::TIME,
+                }),
+                Token::Word(Word {
+                    value: "INDEX".to_string(),
+                    quote_style: None,
+                    keyword: Keyword::INDEX,
+                }),
+            ])))
+        } else {
+            Ok(None)
+        }
+    }
+
     fn parse_optional_table_constraint(&mut self) -> Result<Option<TableConstraint>> {
         let name = if self.parser.parse_keyword(Keyword::CONSTRAINT) {
             Some(
@@ -568,6 +654,7 @@ fn ensure_partition_names_no_duplicate(partitions: &Partitions) -> Result<()> {
 mod tests {
     use std::assert_matches::assert_matches;
 
+    use sqlparser::ast::ColumnOption::NotNull;
     use sqlparser::dialect::GenericDialect;
 
     use super::*;
@@ -977,6 +1064,43 @@ ENGINE=mito";
 
         let result4 = ParserContext::create_with_dialect(sql4, &GenericDialect {});
         assert!(result4.is_err());
+
+        let sql = r"
+CREATE TABLE monitor (
+  host_id    INT,
+  idc        STRING,
+  ts         TIMESTAMP TIME INDEX DEFAULT CURRENT_TIMESTAMP,
+  cpu        DOUBLE DEFAULT 0,
+  memory     DOUBLE,
+  TIME INDEX (ts),
+  PRIMARY KEY (host),
+)
+ENGINE=mito";
+
+        let result = ParserContext::create_with_dialect(sql, &GenericDialect {}).unwrap();
+
+        if let Statement::CreateTable(c) = &result[0] {
+            let tc = c.constraints[0].clone();
+            match tc {
+                TableConstraint::Unique {
+                    name,
+                    columns,
+                    is_primary,
+                } => {
+                    assert_eq!(name.unwrap().to_string(), "__time_index");
+                    assert_eq!(columns.len(), 1);
+                    assert_eq!(&columns[0].value, "ts");
+                    assert!(!is_primary);
+                }
+                _ => panic!("should be time index constraint"),
+            }
+            let ts = c.columns[2].clone();
+            assert_eq!(ts.name.to_string(), "ts");
+            assert!(matches!(ts.options[0].option, ColumnOption::Default(..)));
+            assert_eq!(ts.options[1].option, NotNull);
+        } else {
+            unreachable!("should be create table statement");
+        }
     }
 
     #[test]

tests/cases/standalone/create/create.result

@@ -1,11 +1,23 @@
 CREATE TABLE integers (i BIGINT);
 
-Error: 2000(InvalidSyntax), sql parser error: Expected TIME, found: )
+Error: 1004(InvalidArguments), Missing timestamp column in request
+
+CREATE TABLE integers (i INT TIME INDEX);
+
+Error: 1004(InvalidArguments), Invalid column option, column name: i, error: time index column data type should be timestamp or bigint
+
+CREATE TABLE integers (i BIGINT TIME INDEX NULL);
+
+Error: 1004(InvalidArguments), Invalid column option, column name: i, error: time index column can't be null
 
 CREATE TABLE integers (i BIGINT TIME INDEX);
 
 Affected Rows: 0
 
+CREATE TABLE times (i TIMESTAMP TIME INDEX DEFAULT CURRENT_TIMESTAMP);
+
+Affected Rows: 0
+
 CREATE TABLE IF NOT EXISTS integers (i BIGINT TIME INDEX);
 
 Affected Rows: 0
@@ -20,7 +32,7 @@ Affected Rows: 0
 
 CREATE TABLE test2 (i INTEGER, j BIGINT TIME INDEX NULL);
 
-Error: 2000(InvalidSyntax), sql parser error: Expected NOT, found: NULL
+Error: 1004(InvalidArguments), Invalid column option, column name: j, error: time index column can't be null
 
 CREATE TABLE test2 (i INTEGER, j BIGINT TIME INDEX);
 
@@ -56,6 +68,10 @@ DROP TABLE integers;
 
 Affected Rows: 1
 
+DROP TABLE times;
+
+Affected Rows: 1
+
 DROP TABLE test1;
 
 Affected Rows: 1

tests/cases/standalone/create/create.sql

@@ -1,7 +1,13 @@
 CREATE TABLE integers (i BIGINT);
 
+CREATE TABLE integers (i INT TIME INDEX);
+
+CREATE TABLE integers (i BIGINT TIME INDEX NULL);
+
 CREATE TABLE integers (i BIGINT TIME INDEX);
 
+CREATE TABLE times (i TIMESTAMP TIME INDEX DEFAULT CURRENT_TIMESTAMP);
+
 CREATE TABLE IF NOT EXISTS integers (i BIGINT TIME INDEX);
 
 CREATE TABLE test1 (i INTEGER, j INTEGER);
@@ -20,6 +26,8 @@ DESC TABLE test2;
 
 DROP TABLE integers;
 
+DROP TABLE times;
+
 DROP TABLE test1;
 
 DROP TABLE test2;