Skip to content

Commit

Permalink
feat: only allow timestamp type as time index (#2281)
Browse files Browse the repository at this point in the history 
* feat: only allow timestamp data type as time index

* test: update sqltest cases, todo: need some fixes

* fix: sqlness tests

* fix: forgot adding back cte test

* chore: style
  • Loading branch information
@killme2008 @waynexia
killme2008 authored and waynexia committed Sep 12, 2023
1 parent 6e59340 commit ca3275c
Show file tree
Hide file tree
Showing 62 changed files with 704 additions and 750 deletions.
21 changes: 9 additions & 12 deletions src/datanode/src/sql/create.rs
View file
Expand Up @@ -276,7 +276,7 @@ mod tests {
async fn test_create_table_with_options() {
let sql = r#"
CREATE TABLE demo_table (
"timestamp" BIGINT TIME INDEX,
"timestamp" timestamp TIME INDEX,
"value" DOUBLE,
host STRING PRIMARY KEY
) engine=mito with(regions=1, ttl='7days',write_buffer_size='32MB',some='other');"#;
Expand All @@ -297,7 +297,7 @@ mod tests {
let parsed_stmt = sql_to_statement(
r#"
CREATE TABLE demo_table(
"timestamp" BIGINT TIME INDEX,
"timestamp" timestamp TIME INDEX,
"value" DOUBLE,
host STRING PRIMARY KEY
) engine=mito with(regions=1);"#,
Expand Down Expand Up @@ -335,7 +335,7 @@ mod tests {
pub async fn test_multiple_primary_key_definitions() {
let parsed_stmt = sql_to_statement(
r#"create table demo_table (
"timestamp" BIGINT TIME INDEX,
"timestamp" timestamp TIME INDEX,
"value" DOUBLE,
host STRING PRIMARY KEY,
PRIMARY KEY(host)) engine=mito with(regions=1);"#,
Expand All @@ -350,7 +350,7 @@ mod tests {
pub async fn test_multiple_inline_primary_key_definitions() {
let parsed_stmt = sql_to_statement(
r#"create table demo_table (
"timestamp" BIGINT TIME INDEX,
"timestamp" timestamp TIME INDEX,
"value" DOUBLE PRIMARY KEY,
host STRING PRIMARY KEY) engine=mito with(regions=1);"#,
);
Expand Down Expand Up @@ -379,16 +379,13 @@ mod tests {
/// Constraints specified, not column cannot be found.
#[tokio::test]
pub async fn test_key_not_found() {
let parsed_stmt = sql_to_statement(
r#"create table demo_table(
let sql = r#"create table demo_table(
host string,
TIME INDEX (ts)) engine=mito with(regions=1);"#,
);
TIME INDEX (ts)) engine=mito with(regions=1);"#;

let error =
SqlHandler::create_to_request(42, parsed_stmt, &TableReference::bare("demo_table"))
.unwrap_err();
assert_matches!(error, Error::KeyColumnNotFound { .. });
let res = ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}).unwrap_err();

assert_matches!(res, sql::error::Error::InvalidTimeIndex { .. });
}

#[tokio::test]
Expand Down
75 changes: 39 additions & 36 deletions src/sql/src/parsers/create_parser.rs
View file
Expand Up @@ -359,13 +359,10 @@ impl<'a> ParserContext<'a> {
}
);
ensure!(
matches!(
column.data_type,
DataType::Timestamp(_, _) | DataType::BigInt(_)
),
matches!(column.data_type, DataType::Timestamp(_, _)),
InvalidColumnOptionSnafu {
name: column.name.to_string(),
msg: "time index column data type should be timestamp or bigint",
msg: "time index column data type should be timestamp",
}
);

Expand Down Expand Up @@ -598,8 +595,7 @@ fn validate_time_index(create_table: &CreateTable) -> Result<()> {
} = c
{
if ident.value == TIME_INDEX {
let column_names = columns.iter().map(|c| &c.value).collect::<Vec<_>>();
Some(column_names)
Some(columns)
} else {
None
}
Expand Down Expand Up @@ -627,6 +623,28 @@ 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
.iter()
.find(|c| c.name.value == *time_index_column_ident.value)
.with_context(|| InvalidTimeIndexSnafu {
msg: format!(
"time index column {} not found in columns",
time_index_column_ident
),
})?;

ensure!(
matches!(time_index_column.data_type, DataType::Timestamp(_, _)),
InvalidColumnOptionSnafu {
name: time_index_column.name.to_string(),
msg: "time index column data type should be timestamp",
}
);

Ok(())
}

Expand Down Expand Up @@ -924,7 +942,7 @@ mod tests {
#[test]
fn test_validate_create() {
let sql = r"
CREATE TABLE rcx ( a INT, b STRING, c INT, ts BIGINT TIME INDEX)
CREATE TABLE rcx ( a INT, b STRING, c INT, ts timestamp TIME INDEX)
PARTITION BY RANGE COLUMNS(b, a) (
PARTITION r0 VALUES LESS THAN ('hz', 1000),
PARTITION r1 VALUES LESS THAN ('sh', 2000),
Expand Down Expand Up @@ -1175,7 +1193,7 @@ ENGINE=mito";

assert_ne!(result1, result3);

// BIGINT as time index
// BIGINT can't be time index any more
let sql1 = r"
CREATE TABLE monitor (
host_id INT,
Expand All @@ -1186,27 +1204,12 @@ CREATE TABLE monitor (
PRIMARY KEY (host),
)
ENGINE=mito";
let result1 = ParserContext::create_with_dialect(sql1, &GreptimeDbDialect {}).unwrap();
let result1 = ParserContext::create_with_dialect(sql1, &GreptimeDbDialect {});

if let Statement::CreateTable(c) = &result1[0] {
assert_eq!(c.constraints.len(), 2);
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, "b");
assert!(!is_primary);
}
_ => panic!("should be time index constraint"),
};
} else {
panic!("should be create_table statement");
}
assert!(result1
.unwrap_err()
.to_string()
.contains("time index column data type should be timestamp"));
}

#[test]
Expand Down Expand Up @@ -1385,7 +1388,7 @@ ENGINE=mito";
pub fn test_parse_create_table() {
let sql = r"create table demo(
host string,
ts int64,
ts timestamp,
cpu float64 default 0,
memory float64,
TIME INDEX (ts),
Expand All @@ -1402,7 +1405,7 @@ ENGINE=mito";
assert_eq!(4, c.columns.len());
let columns = &c.columns;
assert_column_def(&columns[0], "host", "STRING");
assert_column_def(&columns[1], "ts", "int64");
assert_column_def(&columns[1], "ts", "TIMESTAMP");
assert_column_def(&columns[2], "cpu", "float64");
assert_column_def(&columns[3], "memory", "float64");
let constraints = &c.constraints;
Expand Down Expand Up @@ -1449,7 +1452,7 @@ ENGINE=mito";
fn test_duplicated_time_index() {
let sql = r"create table demo(
host string,
ts int64 time index,
ts timestamp time index,
t timestamp time index,
cpu float64 default 0,
memory float64,
Expand All @@ -1459,11 +1462,11 @@ ENGINE=mito";
";
let result = ParserContext::create_with_dialect(sql, &GreptimeDbDialect {});
assert!(result.is_err());
assert_matches!(result, Err(crate::error::Error::InvalidColumnOption { .. }));
assert_matches!(result, Err(crate::error::Error::InvalidTimeIndex { .. }));

let sql = r"create table demo(
host string,
ts bigint time index,
ts timestamp time index,
cpu float64 default 0,
t timestamp,
memory float64,
Expand All @@ -1478,7 +1481,7 @@ ENGINE=mito";

#[test]
fn test_invalid_column_name() {
let sql = "create table foo(user string, i bigint time index)";
let sql = "create table foo(user string, i timestamp time index)";
let result = ParserContext::create_with_dialect(sql, &GreptimeDbDialect {});
assert!(result
.unwrap_err()
Expand All @@ -1487,7 +1490,7 @@ ENGINE=mito";

// If column name is quoted, it's valid even same with keyword.
let sql = r#"
create table foo("user" string, i bigint time index)
create table foo("user" string, i timestamp time index)
"#;
let result = ParserContext::create_with_dialect(sql, &GreptimeDbDialect {});
let _ = result.unwrap();
Expand Down
8 changes: 4 additions & 4 deletions src/sql/src/statements/create.rs
View file
Expand Up @@ -229,7 +229,7 @@ mod tests {
fn test_display_create_table() {
let sql = r"create table if not exists demo(
host string,
ts bigint,
ts timestamp,
cpu double default 0,
memory double,
TIME INDEX (ts),
Expand All @@ -253,7 +253,7 @@ mod tests {
r#"
CREATE TABLE IF NOT EXISTS demo (
host STRING,
ts BIGINT,
ts TIMESTAMP,
cpu DOUBLE DEFAULT 0,
memory DOUBLE,
TIME INDEX (ts),
Expand Down Expand Up @@ -284,7 +284,7 @@ WITH(
fn test_display_empty_partition_column() {
let sql = r"create table if not exists demo(
host string,
ts bigint,
ts timestamp,
cpu double default 0,
memory double,
TIME INDEX (ts),
Expand All @@ -301,7 +301,7 @@ WITH(
r#"
CREATE TABLE IF NOT EXISTS demo (
host STRING,
ts BIGINT,
ts TIMESTAMP,
cpu DOUBLE DEFAULT 0,
memory DOUBLE,
TIME INDEX (ts),
Expand Down

0 comments on commit ca3275c

Please sign in to comment.