Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: only allow timestamp type as time index #2281

Merged
merged 5 commits into from Aug 30, 2023
Merged

feat: only allow timestamp type as time index #2281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ff334ef
feat: only allow timestamp data type as time index
killme2008 Aug 29, 2023
7a38d09
test: update sqltest cases, todo: need some fixes
killme2008 Aug 29, 2023
05940ee
fix: sqlness tests
killme2008 Aug 30, 2023
169a2d2
fix: forgot adding back cte test
killme2008 Aug 30, 2023
01081c9
chore: style
killme2008 Aug 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 9 additions & 12 deletions src/datanode/src/sql/create.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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