From 8d446ed74100724e74376186e76349b3bfbbbf7c Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Fri, 25 Aug 2023 02:25:21 -0500 Subject: [PATCH] fix: quote ident on rendered SQL (#2248) * fix: quote ident on rendered SQL Signed-off-by: Ruihang Xia * read quote style from query context Signed-off-by: Ruihang Xia * update sqlness result Signed-off-by: Ruihang Xia --------- Signed-off-by: Ruihang Xia --- src/datanode/src/instance/sql.rs | 3 +- src/frontend/src/instance/distributed.rs | 13 +++- src/query/src/sql.rs | 23 +++++- src/query/src/sql/show_create_table.rs | 50 +++++++------ src/servers/src/error.rs | 7 +- src/sql/src/error.rs | 2 +- src/sql/src/statements/create.rs | 9 +++ tests-integration/src/tests/instance_test.rs | 50 +++++++------ .../cases/distributed/show/show_create.result | 72 +++++++++---------- .../cases/standalone/show/show_create.result | 38 +++++----- 10 files changed, 154 insertions(+), 113 deletions(-) diff --git a/src/datanode/src/instance/sql.rs b/src/datanode/src/instance/sql.rs index df5b3eb93c..5dda39a601 100644 --- a/src/datanode/src/instance/sql.rs +++ b/src/datanode/src/instance/sql.rs @@ -141,7 +141,8 @@ impl Instance { let table_ref = TableReference::full(&catalog, &schema, &table); let table = self.sql_handler.get_table(&table_ref).await?; - query::sql::show_create_table(table, None).context(ExecuteStatementSnafu) + query::sql::show_create_table(table, None, query_ctx.clone()) + .context(ExecuteStatementSnafu) } Statement::TruncateTable(truncate_table) => { let (catalog_name, schema_name, table_name) = diff --git a/src/frontend/src/instance/distributed.rs b/src/frontend/src/instance/distributed.rs index 7bb26d59c0..326bcd08e3 100644 --- a/src/frontend/src/instance/distributed.rs +++ b/src/frontend/src/instance/distributed.rs @@ -399,7 +399,8 @@ impl DistInstance { .context(TableNotFoundSnafu { table_name: &table })?; let table_name = TableName::new(catalog, schema, table); - self.show_create_table(table_name, table_ref).await + self.show_create_table(table_name, table_ref, query_ctx.clone()) + .await } Statement::TruncateTable(stmt) => { let (catalog, schema, table) = @@ -416,7 +417,12 @@ impl DistInstance { } } - async fn show_create_table(&self, table_name: TableName, table: TableRef) -> Result { + async fn show_create_table( + &self, + table_name: TableName, + table: TableRef, + query_ctx: QueryContextRef, + ) -> Result { let partitions = self .catalog_manager .partition_manager() @@ -428,7 +434,8 @@ impl DistInstance { let partitions = create_partitions_stmt(partitions)?; - query::sql::show_create_table(table, partitions).context(error::ExecuteStatementSnafu) + query::sql::show_create_table(table, partitions, query_ctx) + .context(error::ExecuteStatementSnafu) } /// Handles distributed database creation diff --git a/src/query/src/sql.rs b/src/query/src/sql.rs index f51d5c3b13..6f5fafc632 100644 --- a/src/query/src/sql.rs +++ b/src/query/src/sql.rs @@ -185,11 +185,28 @@ pub async fn show_tables( } } -pub fn show_create_table(table: TableRef, partitions: Option) -> Result { +pub fn show_create_table( + table: TableRef, + partitions: Option, + query_ctx: QueryContextRef, +) -> Result { let table_info = table.table_info(); let table_name = &table_info.name; - let mut stmt = show_create_table::create_table_stmt(&table_info)?; - stmt.partitions = partitions; + + // Default to double quote and fallback to back quote + let quote_style = if query_ctx.sql_dialect().is_delimited_identifier_start('"') { + '"' + } else if query_ctx.sql_dialect().is_delimited_identifier_start('\'') { + '\'' + } else { + '`' + }; + + let mut stmt = show_create_table::create_table_stmt(&table_info, quote_style)?; + stmt.partitions = partitions.map(|mut p| { + p.set_quote(quote_style); + p + }); let sql = format!("{}", stmt); let columns = vec![ Arc::new(StringVector::from(vec![table_name.clone()])) as _, diff --git a/src/query/src/sql/show_create_table.rs b/src/query/src/sql/show_create_table.rs index 5f48d7e1c2..afb5e2927e 100644 --- a/src/query/src/sql/show_create_table.rs +++ b/src/query/src/sql/show_create_table.rs @@ -20,7 +20,7 @@ use datatypes::schema::{ColumnDefaultConstraint, ColumnSchema, SchemaRef, COMMEN use humantime::format_duration; use snafu::ResultExt; use sql::ast::{ - ColumnDef, ColumnOption, ColumnOptionDef, Expr, ObjectName, SqlOption, TableConstraint, + ColumnDef, ColumnOption, ColumnOptionDef, Expr, Ident, ObjectName, SqlOption, TableConstraint, Value as SqlValue, }; use sql::dialect::GreptimeDbDialect; @@ -90,7 +90,7 @@ fn column_option_def(option: ColumnOption) -> ColumnOptionDef { ColumnOptionDef { name: None, option } } -fn create_column_def(column_schema: &ColumnSchema) -> Result { +fn create_column_def(column_schema: &ColumnSchema, quote_style: char) -> Result { let name = &column_schema.name; let mut options = Vec::with_capacity(2); @@ -119,7 +119,7 @@ fn create_column_def(column_schema: &ColumnSchema) -> Result { } Ok(ColumnDef { - name: name[..].into(), + name: Ident::with_quote(quote_style, name), data_type: statements::concrete_data_type_to_sql_data_type(&column_schema.data_type) .with_context(|_| ConvertSqlTypeSnafu { datatype: column_schema.data_type.clone(), @@ -129,20 +129,24 @@ fn create_column_def(column_schema: &ColumnSchema) -> Result { }) } -fn create_table_constraints(schema: &SchemaRef, table_meta: &TableMeta) -> Vec { +fn create_table_constraints( + schema: &SchemaRef, + table_meta: &TableMeta, + quote_style: char, +) -> Vec { let mut constraints = Vec::with_capacity(2); if let Some(timestamp_column) = schema.timestamp_column() { let column_name = ×tamp_column.name; constraints.push(TableConstraint::Unique { name: Some(TIME_INDEX.into()), - columns: vec![column_name[..].into()], + columns: vec![Ident::with_quote(quote_style, column_name)], is_primary: false, }); } if !table_meta.primary_key_indices.is_empty() { let columns = table_meta .row_key_column_names() - .map(|name| name[..].into()) + .map(|name| Ident::with_quote(quote_style, name)) .collect(); constraints.push(TableConstraint::Unique { name: None, @@ -155,7 +159,7 @@ fn create_table_constraints(schema: &SchemaRef, table_meta: &TableMeta) -> Vec Result { +pub fn create_table_stmt(table_info: &TableInfoRef, quote_style: char) -> Result { let table_meta = &table_info.meta; let table_name = &table_info.name; let schema = &table_info.meta.schema; @@ -163,15 +167,15 @@ pub fn create_table_stmt(table_info: &TableInfoRef) -> Result { let columns = schema .column_schemas() .iter() - .map(create_column_def) + .map(|c| create_column_def(c, quote_style)) .collect::>>()?; - let constraints = create_table_constraints(schema, table_meta); + let constraints = create_table_constraints(schema, table_meta, quote_style); Ok(CreateTable { if_not_exists: true, table_id: table_info.ident.table_id, - name: ObjectName(vec![table_name[..].into()]), + name: ObjectName(vec![Ident::with_quote(quote_style, table_name)]), columns, engine: table_meta.engine.clone(), constraints, @@ -246,19 +250,19 @@ mod tests { .unwrap(), ); - let stmt = create_table_stmt(&info).unwrap(); + let stmt = create_table_stmt(&info, '"').unwrap(); let sql = format!("\n{}", stmt); assert_eq!( r#" -CREATE TABLE IF NOT EXISTS system_metrics ( - id INT UNSIGNED NULL, - host STRING NULL, - cpu DOUBLE NULL, - disk FLOAT NULL, - ts TIMESTAMP(3) NOT NULL DEFAULT current_timestamp(), - TIME INDEX (ts), - PRIMARY KEY (id, host) +CREATE TABLE IF NOT EXISTS "system_metrics" ( + "id" INT UNSIGNED NULL, + "host" STRING NULL, + "cpu" DOUBLE NULL, + "disk" FLOAT NULL, + "ts" TIMESTAMP(3) NOT NULL DEFAULT current_timestamp(), + TIME INDEX ("ts"), + PRIMARY KEY ("id", "host") ) ENGINE=mito WITH( @@ -315,14 +319,14 @@ WITH( .unwrap(), ); - let stmt = create_table_stmt(&info).unwrap(); + let stmt = create_table_stmt(&info, '"').unwrap(); let sql = format!("\n{}", stmt); assert_eq!( r#" -CREATE EXTERNAL TABLE IF NOT EXISTS system_metrics ( - host STRING NULL, - cpu DOUBLE NULL, +CREATE EXTERNAL TABLE IF NOT EXISTS "system_metrics" ( + "host" STRING NULL, + "cpu" DOUBLE NULL, ) ENGINE=file diff --git a/src/servers/src/error.rs b/src/servers/src/error.rs index e1bb625453..1e583c2c2f 100644 --- a/src/servers/src/error.rs +++ b/src/servers/src/error.rs @@ -65,7 +65,12 @@ pub enum Error { source: std::io::Error, }, - #[snafu(display("Failed to execute query: {}, source: {}", query, source))] + #[snafu(display( + "Failed to execute query, source: {}, query: {}, location: {}", + source, + query, + location + ))] ExecuteQuery { query: String, location: Location, diff --git a/src/sql/src/error.rs b/src/sql/src/error.rs index a3c05694ac..1ced9be3ce 100644 --- a/src/sql/src/error.rs +++ b/src/sql/src/error.rs @@ -57,7 +57,7 @@ pub enum Error { UnsupportedDefaultValue { column_name: String, expr: Expr }, // Syntax error from sql parser. - #[snafu(display("Syntax error, sql: {}, source: {}", sql, source))] + #[snafu(display("Syntax error, source: {}, sql: {}", source, sql))] Syntax { sql: String, source: ParserError }, #[snafu(display("Missing time index constraint"))] diff --git a/src/sql/src/statements/create.rs b/src/sql/src/statements/create.rs index 5ae756ebfb..870b976edf 100644 --- a/src/sql/src/statements/create.rs +++ b/src/sql/src/statements/create.rs @@ -130,6 +130,15 @@ pub struct Partitions { pub entries: Vec, } +impl Partitions { + /// set quotes to all [Ident]s from column list + pub fn set_quote(&mut self, quote_style: char) { + self.column_list + .iter_mut() + .for_each(|c| c.quote_style = Some(quote_style)); + } +} + #[derive(Debug, PartialEq, Eq, Clone)] pub struct PartitionEntry { pub name: Ident, diff --git a/tests-integration/src/tests/instance_test.rs b/tests-integration/src/tests/instance_test.rs index 71c3dc7cda..ace4b9afed 100644 --- a/tests-integration/src/tests/instance_test.rs +++ b/tests-integration/src/tests/instance_test.rs @@ -109,18 +109,17 @@ PARTITION BY RANGE COLUMNS (ts) ( let output = execute_sql(&frontend, "show create table demo").await; let expected = if instance.is_distributed_mode() { - "\ -+-------+----------------------------------------------------------+ + r#"+-------+----------------------------------------------------------+ | Table | Create Table | +-------+----------------------------------------------------------+ -| demo | CREATE TABLE IF NOT EXISTS demo ( | -| | host STRING NULL, | -| | cpu DOUBLE NULL, | -| | memory DOUBLE NULL, | -| | ts BIGINT NOT NULL, | -| | TIME INDEX (ts) | +| demo | CREATE TABLE IF NOT EXISTS "demo" ( | +| | "host" STRING NULL, | +| | "cpu" DOUBLE NULL, | +| | "memory" DOUBLE NULL, | +| | "ts" BIGINT NOT NULL, | +| | TIME INDEX ("ts") | | | ) | -| | PARTITION BY RANGE COLUMNS (ts) ( | +| | PARTITION BY RANGE COLUMNS ("ts") ( | | | PARTITION r0 VALUES LESS THAN (1), | | | PARTITION r1 VALUES LESS THAN (10), | | | PARTITION r2 VALUES LESS THAN (100), | @@ -130,24 +129,23 @@ PARTITION BY RANGE COLUMNS (ts) ( | | WITH( | | | regions = 4 | | | ) | -+-------+----------------------------------------------------------+" ++-------+----------------------------------------------------------+"# } else { - "\ -+-------+-----------------------------------+ -| Table | Create Table | -+-------+-----------------------------------+ -| demo | CREATE TABLE IF NOT EXISTS demo ( | -| | host STRING NULL, | -| | cpu DOUBLE NULL, | -| | memory DOUBLE NULL, | -| | ts BIGINT NOT NULL, | -| | TIME INDEX (ts) | -| | ) | -| | ENGINE=mito | -| | WITH( | -| | regions = 1 | -| | ) | -+-------+-----------------------------------+" + r#"+-------+-------------------------------------+ +| Table | Create Table | ++-------+-------------------------------------+ +| demo | CREATE TABLE IF NOT EXISTS "demo" ( | +| | "host" STRING NULL, | +| | "cpu" DOUBLE NULL, | +| | "memory" DOUBLE NULL, | +| | "ts" BIGINT NOT NULL, | +| | TIME INDEX ("ts") | +| | ) | +| | ENGINE=mito | +| | WITH( | +| | regions = 1 | +| | ) | ++-------+-------------------------------------+"# }; check_output_stream(output, expected).await; diff --git a/tests/cases/distributed/show/show_create.result b/tests/cases/distributed/show/show_create.result index 42e7674945..2789ae27cc 100644 --- a/tests/cases/distributed/show/show_create.result +++ b/tests/cases/distributed/show/show_create.result @@ -19,29 +19,29 @@ Affected Rows: 0 SHOW CREATE TABLE system_metrics; -+----------------+----------------------------------------------------------+ -| Table | Create Table | -+----------------+----------------------------------------------------------+ -| system_metrics | CREATE TABLE IF NOT EXISTS system_metrics ( | -| | id INT UNSIGNED NULL, | -| | host STRING NULL, | -| | cpu DOUBLE NULL, | -| | disk FLOAT NULL, | -| | n INT NULL, | -| | ts TIMESTAMP(3) NOT NULL DEFAULT current_timestamp(), | -| | TIME INDEX (ts), | -| | PRIMARY KEY (id, host) | -| | ) | -| | PARTITION BY RANGE COLUMNS (n) ( | -| | PARTITION r0 VALUES LESS THAN (5), | -| | PARTITION r1 VALUES LESS THAN (9), | -| | PARTITION r2 VALUES LESS THAN (MAXVALUE) | -| | ) | -| | ENGINE=mito | -| | WITH( | -| | regions = 3 | -| | ) | -+----------------+----------------------------------------------------------+ ++----------------+-----------------------------------------------------------+ +| Table | Create Table | ++----------------+-----------------------------------------------------------+ +| system_metrics | CREATE TABLE IF NOT EXISTS "system_metrics" ( | +| | "id" INT UNSIGNED NULL, | +| | "host" STRING NULL, | +| | "cpu" DOUBLE NULL, | +| | "disk" FLOAT NULL, | +| | "n" INT NULL, | +| | "ts" TIMESTAMP(3) NOT NULL DEFAULT current_timestamp(), | +| | TIME INDEX ("ts"), | +| | PRIMARY KEY ("id", "host") | +| | ) | +| | PARTITION BY RANGE COLUMNS ("n") ( | +| | PARTITION r0 VALUES LESS THAN (5), | +| | PARTITION r1 VALUES LESS THAN (9), | +| | PARTITION r2 VALUES LESS THAN (MAXVALUE) | +| | ) | +| | ENGINE=mito | +| | WITH( | +| | regions = 3 | +| | ) | ++----------------+-----------------------------------------------------------+ DROP TABLE system_metrics; @@ -55,19 +55,19 @@ Affected Rows: 0 show create table table_without_partition; -+-------------------------+---------------------------------------------------------+ -| Table | Create Table | -+-------------------------+---------------------------------------------------------+ -| table_without_partition | CREATE TABLE IF NOT EXISTS table_without_partition ( | -| | ts TIMESTAMP(3) NOT NULL DEFAULT current_timestamp(), | -| | TIME INDEX (ts) | -| | ) | -| | | -| | ENGINE=mito | -| | WITH( | -| | regions = 1 | -| | ) | -+-------------------------+---------------------------------------------------------+ ++-------------------------+-----------------------------------------------------------+ +| Table | Create Table | ++-------------------------+-----------------------------------------------------------+ +| table_without_partition | CREATE TABLE IF NOT EXISTS "table_without_partition" ( | +| | "ts" TIMESTAMP(3) NOT NULL DEFAULT current_timestamp(), | +| | TIME INDEX ("ts") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | regions = 1 | +| | ) | ++-------------------------+-----------------------------------------------------------+ drop table table_without_partition; diff --git a/tests/cases/standalone/show/show_create.result b/tests/cases/standalone/show/show_create.result index 763bf3e9d6..d370d2646d 100644 --- a/tests/cases/standalone/show/show_create.result +++ b/tests/cases/standalone/show/show_create.result @@ -17,25 +17,25 @@ Affected Rows: 0 SHOW CREATE TABLE system_metrics; -+----------------+---------------------------------------------------------+ -| Table | Create Table | -+----------------+---------------------------------------------------------+ -| system_metrics | CREATE TABLE IF NOT EXISTS system_metrics ( | -| | id INT UNSIGNED NULL, | -| | host STRING NULL, | -| | cpu DOUBLE NULL COMMENT 'cpu', | -| | disk FLOAT NULL, | -| | ts TIMESTAMP(3) NOT NULL DEFAULT current_timestamp(), | -| | TIME INDEX (ts), | -| | PRIMARY KEY (id, host) | -| | ) | -| | ENGINE=mito | -| | WITH( | -| | regions = 1, | -| | ttl = '7days', | -| | write_buffer_size = '1.0KiB' | -| | ) | -+----------------+---------------------------------------------------------+ ++----------------+-----------------------------------------------------------+ +| Table | Create Table | ++----------------+-----------------------------------------------------------+ +| system_metrics | CREATE TABLE IF NOT EXISTS "system_metrics" ( | +| | "id" INT UNSIGNED NULL, | +| | "host" STRING NULL, | +| | "cpu" DOUBLE NULL COMMENT 'cpu', | +| | "disk" FLOAT NULL, | +| | "ts" TIMESTAMP(3) NOT NULL DEFAULT current_timestamp(), | +| | TIME INDEX ("ts"), | +| | PRIMARY KEY ("id", "host") | +| | ) | +| | ENGINE=mito | +| | WITH( | +| | regions = 1, | +| | ttl = '7days', | +| | write_buffer_size = '1.0KiB' | +| | ) | ++----------------+-----------------------------------------------------------+ DROP TABLE system_metrics;