From 2d77df3dc421e5540f4eecc19ed42f3d79f6eaf3 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Mon, 18 Sep 2023 14:17:28 +0800 Subject: [PATCH 1/2] feat: add comment field to ColumnDef Signed-off-by: Ruihang Xia --- Cargo.lock | 2 +- Cargo.toml | 2 +- benchmarks/src/bin/nyc-taxi.rs | 19 +++++++++++++++++++ src/api/src/v1/column_def.rs | 22 ++++++++++++++++------ src/client/examples/logical.rs | 3 +++ src/common/grpc-expr/src/alter.rs | 3 +++ src/common/grpc-expr/src/util.rs | 2 ++ src/common/meta/src/ddl/create_table.rs | 1 + src/meta-srv/src/procedure/tests.rs | 10 ++++++++++ src/operator/src/expr_factory.rs | 8 +++++++- src/sql/src/statements.rs | 1 + src/store-api/src/region_request.rs | 1 + src/store-api/src/storage/requests.rs | 2 ++ 13 files changed, 67 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 61cc09aba6b5..51073e8869bb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4098,7 +4098,7 @@ checksum = "d2fabcfbdc87f4758337ca535fb41a6d701b65693ce38287d856d1674551ec9b" [[package]] name = "greptime-proto" version = "0.1.0" -source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=e81a60e817a348ee5b7dfbd991f86d35cd068ce5#e81a60e817a348ee5b7dfbd991f86d35cd068ce5" +source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=637c54338f0cd0a333fd67bcb37d79dabb014453#637c54338f0cd0a333fd67bcb37d79dabb014453" dependencies = [ "prost", "serde", diff --git a/Cargo.toml b/Cargo.toml index 074993900450..533bedf27653 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -80,7 +80,7 @@ datafusion-substrait = { git = "https://github.com/waynexia/arrow-datafusion.git derive_builder = "0.12" futures = "0.3" futures-util = "0.3" -greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "e81a60e817a348ee5b7dfbd991f86d35cd068ce5" } +greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "637c54338f0cd0a333fd67bcb37d79dabb014453" } humantime-serde = "1.1" itertools = "0.10" lazy_static = "1.4" diff --git a/benchmarks/src/bin/nyc-taxi.rs b/benchmarks/src/bin/nyc-taxi.rs index 648541db1e24..5f14c655512b 100644 --- a/benchmarks/src/bin/nyc-taxi.rs +++ b/benchmarks/src/bin/nyc-taxi.rs @@ -256,6 +256,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Tag as i32, + comment: String::new(), }, ColumnDef { name: "tpep_pickup_datetime".to_string(), @@ -263,6 +264,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Timestamp as i32, + comment: String::new(), }, ColumnDef { name: "tpep_dropoff_datetime".to_string(), @@ -270,6 +272,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ColumnDef { name: "passenger_count".to_string(), @@ -277,6 +280,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ColumnDef { name: "trip_distance".to_string(), @@ -284,6 +288,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ColumnDef { name: "RatecodeID".to_string(), @@ -291,6 +296,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ColumnDef { name: "store_and_fwd_flag".to_string(), @@ -298,6 +304,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ColumnDef { name: "PULocationID".to_string(), @@ -305,6 +312,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ColumnDef { name: "DOLocationID".to_string(), @@ -312,6 +320,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ColumnDef { name: "payment_type".to_string(), @@ -319,6 +328,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ColumnDef { name: "fare_amount".to_string(), @@ -326,6 +336,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ColumnDef { name: "extra".to_string(), @@ -333,6 +344,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ColumnDef { name: "mta_tax".to_string(), @@ -340,6 +352,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ColumnDef { name: "tip_amount".to_string(), @@ -347,6 +360,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ColumnDef { name: "tolls_amount".to_string(), @@ -354,6 +368,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ColumnDef { name: "improvement_surcharge".to_string(), @@ -361,6 +376,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ColumnDef { name: "total_amount".to_string(), @@ -368,6 +384,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ColumnDef { name: "congestion_surcharge".to_string(), @@ -375,6 +392,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ColumnDef { name: "airport_fee".to_string(), @@ -382,6 +400,7 @@ fn create_table_expr() -> CreateTableExpr { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ], time_index: "tpep_pickup_datetime".to_string(), diff --git a/src/api/src/v1/column_def.rs b/src/api/src/v1/column_def.rs index bd29d1d98605..7a812d005fae 100644 --- a/src/api/src/v1/column_def.rs +++ b/src/api/src/v1/column_def.rs @@ -12,7 +12,9 @@ // See the License for the specific language governing permissions and // limitations under the License. -use datatypes::schema::{ColumnDefaultConstraint, ColumnSchema}; +use std::collections::HashMap; + +use datatypes::schema::{ColumnDefaultConstraint, ColumnSchema, COMMENT_KEY}; use snafu::ResultExt; use crate::error::{self, Result}; @@ -34,9 +36,17 @@ pub fn try_as_column_schema(column_def: &ColumnDef) -> Result { ) }; - ColumnSchema::new(&column_def.name, data_type.into(), column_def.is_nullable) - .with_default_constraint(constraint) - .context(error::InvalidColumnDefaultConstraintSnafu { - column: &column_def.name, - }) + let mut metadata = HashMap::new(); + if !column_def.comment.is_empty() { + metadata.insert(COMMENT_KEY.to_string(), column_def.comment.clone()); + } + + Ok( + ColumnSchema::new(&column_def.name, data_type.into(), column_def.is_nullable) + .with_default_constraint(constraint) + .context(error::InvalidColumnDefaultConstraintSnafu { + column: &column_def.name, + })? + .with_metadata(metadata), + ) } diff --git a/src/client/examples/logical.rs b/src/client/examples/logical.rs index d13c4844c072..46f1b1e9e6ec 100644 --- a/src/client/examples/logical.rs +++ b/src/client/examples/logical.rs @@ -45,6 +45,7 @@ async fn run() { is_nullable: false, default_constraint: vec![], semantic_type: SemanticType::Timestamp as i32, + comment: String::new(), }, ColumnDef { name: "key".to_string(), @@ -52,6 +53,7 @@ async fn run() { is_nullable: false, default_constraint: vec![], semantic_type: SemanticType::Tag as i32, + comment: String::new(), }, ColumnDef { name: "value".to_string(), @@ -59,6 +61,7 @@ async fn run() { is_nullable: false, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ], time_index: "timestamp".to_string(), diff --git a/src/common/grpc-expr/src/alter.rs b/src/common/grpc-expr/src/alter.rs index 8e6ef3bc9af2..462eefde1a27 100644 --- a/src/common/grpc-expr/src/alter.rs +++ b/src/common/grpc-expr/src/alter.rs @@ -157,6 +157,7 @@ mod tests { is_nullable: false, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }), location: None, }], @@ -197,6 +198,7 @@ mod tests { is_nullable: false, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }), location: Some(Location { location_type: LocationType::First.into(), @@ -210,6 +212,7 @@ mod tests { is_nullable: false, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }), location: Some(Location { location_type: LocationType::After.into(), diff --git a/src/common/grpc-expr/src/util.rs b/src/common/grpc-expr/src/util.rs index 641b4022472e..337a0a16e6e2 100644 --- a/src/common/grpc-expr/src/util.rs +++ b/src/common/grpc-expr/src/util.rs @@ -120,6 +120,7 @@ pub fn build_create_table_expr( is_nullable, default_constraint: vec![], semantic_type, + comment: String::new(), }; column_defs.push(column_def); } @@ -159,6 +160,7 @@ pub fn extract_new_columns( is_nullable: true, default_constraint: vec![], semantic_type: expr.semantic_type, + comment: String::new(), }); AddColumn { column_def, diff --git a/src/common/meta/src/ddl/create_table.rs b/src/common/meta/src/ddl/create_table.rs index 904dcc9148e3..ee955fdb9131 100644 --- a/src/common/meta/src/ddl/create_table.rs +++ b/src/common/meta/src/ddl/create_table.rs @@ -130,6 +130,7 @@ impl CreateTableProcedure { is_nullable: c.is_nullable, default_constraint: c.default_constraint.clone(), semantic_type: semantic_type as i32, + comment: String::new(), }), column_id: i as u32, } diff --git a/src/meta-srv/src/procedure/tests.rs b/src/meta-srv/src/procedure/tests.rs index 9f11a6ef92ae..3979e2876419 100644 --- a/src/meta-srv/src/procedure/tests.rs +++ b/src/meta-srv/src/procedure/tests.rs @@ -52,6 +52,7 @@ fn create_table_task() -> CreateTableTask { is_nullable: false, default_constraint: vec![], semantic_type: SemanticType::Timestamp as i32, + comment: String::new(), }, PbColumnDef { name: "my_tag1".to_string(), @@ -59,6 +60,7 @@ fn create_table_task() -> CreateTableTask { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Tag as i32, + comment: String::new(), }, PbColumnDef { name: "my_tag2".to_string(), @@ -66,6 +68,7 @@ fn create_table_task() -> CreateTableTask { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Tag as i32, + comment: String::new(), }, PbColumnDef { name: "my_field_column".to_string(), @@ -73,6 +76,7 @@ fn create_table_task() -> CreateTableTask { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }, ], time_index: "ts".to_string(), @@ -108,6 +112,7 @@ fn test_create_region_request_template() { is_nullable: false, default_constraint: vec![], semantic_type: SemanticType::Timestamp as i32, + comment: String::new(), }), column_id: 0, }, @@ -118,6 +123,7 @@ fn test_create_region_request_template() { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Tag as i32, + comment: String::new(), }), column_id: 1, }, @@ -128,6 +134,7 @@ fn test_create_region_request_template() { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Tag as i32, + comment: String::new(), }), column_id: 2, }, @@ -138,6 +145,7 @@ fn test_create_region_request_template() { is_nullable: true, default_constraint: vec![], semantic_type: SemanticType::Field as i32, + comment: String::new(), }), column_id: 3, }, @@ -278,6 +286,7 @@ fn test_create_alter_region_request() { is_nullable: true, default_constraint: b"hello".to_vec(), semantic_type: SemanticType::Tag as i32, + comment: String::new(), }), location: Some(AddColumnLocation { location_type: LocationType::After as i32, @@ -312,6 +321,7 @@ fn test_create_alter_region_request() { is_nullable: true, default_constraint: b"hello".to_vec(), semantic_type: SemanticType::Tag as i32, + comment: String::new() }), column_id: 3, }), diff --git a/src/operator/src/expr_factory.rs b/src/operator/src/expr_factory.rs index 82a487050b29..16390f69b775 100644 --- a/src/operator/src/expr_factory.rs +++ b/src/operator/src/expr_factory.rs @@ -22,7 +22,7 @@ use api::v1::{ }; use common_error::ext::BoxedError; use common_grpc_expr::util::ColumnExpr; -use datatypes::schema::ColumnSchema; +use datatypes::schema::{ColumnSchema, COMMENT_KEY}; use file_table_engine::table::immutable::ImmutableFileTableOptions; use query::sql::prepare_immutable_file_table_files_and_schema; use session::context::QueryContextRef; @@ -271,6 +271,11 @@ pub fn column_schemas_to_defs( } else { SemanticType::Field } as i32; + let comment = schema + .metadata() + .get(COMMENT_KEY) + .cloned() + .unwrap_or_default(); Ok(api::v1::ColumnDef { name: schema.name.clone(), @@ -287,6 +292,7 @@ pub fn column_schemas_to_defs( } }, semantic_type, + comment, }) }) .collect() diff --git a/src/sql/src/statements.rs b/src/sql/src/statements.rs index 1510335c77cd..de54d729809a 100644 --- a/src/sql/src/statements.rs +++ b/src/sql/src/statements.rs @@ -343,6 +343,7 @@ pub fn sql_column_def_to_grpc_column_def(col: &ColumnDef) -> Result Date: Mon, 18 Sep 2023 14:17:41 +0800 Subject: [PATCH 2/2] fix sqlness case Signed-off-by: Ruihang Xia --- tests/cases/distributed/show/show_create.result | 4 ++-- tests/cases/distributed/show/show_create.sql | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/cases/distributed/show/show_create.result b/tests/cases/distributed/show/show_create.result index 8dde89eb4a74..86e3687ed5b8 100644 --- a/tests/cases/distributed/show/show_create.result +++ b/tests/cases/distributed/show/show_create.result @@ -2,7 +2,7 @@ CREATE TABLE system_metrics ( id INT UNSIGNED, host STRING, cpu DOUBLE, - disk FLOAT, + disk FLOAT COMMENT 'comment', ts TIMESTAMP NOT NULL DEFAULT current_timestamp(), TIME INDEX (ts), PRIMARY KEY (id, host) @@ -29,7 +29,7 @@ SHOW CREATE TABLE system_metrics; | | "id" INT UNSIGNED NULL, | | | "host" STRING NULL, | | | "cpu" DOUBLE NULL, | -| | "disk" FLOAT NULL, | +| | "disk" FLOAT NULL COMMENT 'comment', | | | "ts" TIMESTAMP(3) NOT NULL DEFAULT current_timestamp(), | | | TIME INDEX ("ts"), | | | PRIMARY KEY ("id", "host") | diff --git a/tests/cases/distributed/show/show_create.sql b/tests/cases/distributed/show/show_create.sql index ca429638cf9d..56b56baadcbc 100644 --- a/tests/cases/distributed/show/show_create.sql +++ b/tests/cases/distributed/show/show_create.sql @@ -2,7 +2,7 @@ CREATE TABLE system_metrics ( id INT UNSIGNED, host STRING, cpu DOUBLE, - disk FLOAT, + disk FLOAT COMMENT 'comment', ts TIMESTAMP NOT NULL DEFAULT current_timestamp(), TIME INDEX (ts), PRIMARY KEY (id, host)