From 95fb701be53419109454576aea6adf62274b3120 Mon Sep 17 00:00:00 2001 From: zhyass <34016424+zhyass@users.noreply.github.com> Date: Sun, 5 Jun 2022 13:03:37 +0800 Subject: [PATCH] fix review comment --- common/meta/app/src/schema/table.rs | 12 +++++++----- .../src/table_from_to_protobuf_impl.rs | 2 ++ common/proto-conv/tests/it/proto_conv.rs | 18 ++++++++++-------- common/protos/proto/metadata.proto | 5 ++++- .../interpreter_table_show_create.rs | 2 +- query/src/sql/planner/binder/ddl/table.rs | 4 ++-- .../sql/statements/statement_create_table.rs | 2 +- query/src/storages/fuse/fuse_table.rs | 6 +++--- query/src/storages/storage_table.rs | 2 +- 9 files changed, 31 insertions(+), 22 deletions(-) diff --git a/common/meta/app/src/schema/table.rs b/common/meta/app/src/schema/table.rs index 28cc361cbb6a..77afea85464c 100644 --- a/common/meta/app/src/schema/table.rs +++ b/common/meta/app/src/schema/table.rs @@ -174,6 +174,7 @@ pub struct TableMeta { pub engine: String, pub engine_options: BTreeMap, pub options: BTreeMap, + pub cluster_key: Option, // The vector of cluster keys. pub cluster_keys: Vec, // The default cluster keys id. @@ -240,6 +241,7 @@ impl Default for TableMeta { engine: "".to_string(), engine_options: BTreeMap::new(), options: BTreeMap::new(), + cluster_key: None, cluster_keys: vec![], default_cluster_key_id: None, created_on: Default::default(), @@ -252,15 +254,15 @@ impl Default for TableMeta { } impl TableMeta { - pub fn set_cluster_keys_meta(mut self, cluster_key: String) -> Self { - self.cluster_keys.push(cluster_key); + pub fn push_cluster_key(mut self, cluster_key: String) -> Self { + self.cluster_keys.push(cluster_key.clone()); + self.cluster_key = Some(cluster_key); self.default_cluster_key_id = Some(self.cluster_keys.len() as u32 - 1); self } - pub fn cluster_keys(&self) -> Option<(u32, String)> { - self.default_cluster_key_id - .map(|id| (id, self.cluster_keys[id as usize].clone())) + pub fn cluster_key(&self) -> Option<(u32, String)> { + self.default_cluster_key_id.zip(self.cluster_key.clone()) } } diff --git a/common/proto-conv/src/table_from_to_protobuf_impl.rs b/common/proto-conv/src/table_from_to_protobuf_impl.rs index 516c4b1ce82a..f15bd9beb4a3 100644 --- a/common/proto-conv/src/table_from_to_protobuf_impl.rs +++ b/common/proto-conv/src/table_from_to_protobuf_impl.rs @@ -126,6 +126,7 @@ impl FromToProto for mt::TableMeta { engine: p.engine, engine_options: p.engine_options, options: p.options, + cluster_key: p.cluster_key, cluster_keys: p.cluster_keys, default_cluster_key_id: p.default_cluster_key_id, created_on: DateTime::::from_pb(p.created_on)?, @@ -151,6 +152,7 @@ impl FromToProto for mt::TableMeta { engine: self.engine.clone(), engine_options: self.engine_options.clone(), options: self.options.clone(), + cluster_key: self.cluster_key.clone(), cluster_keys: self.cluster_keys.clone(), default_cluster_key_id: self.default_cluster_key_id, created_on: self.created_on.to_pb()?, diff --git a/common/proto-conv/tests/it/proto_conv.rs b/common/proto-conv/tests/it/proto_conv.rs index 4e077cb370d5..d045a4fb59de 100644 --- a/common/proto-conv/tests/it/proto_conv.rs +++ b/common/proto-conv/tests/it/proto_conv.rs @@ -108,6 +108,7 @@ fn new_table_info() -> mt::TableInfo { engine: "44".to_string(), engine_options: btreemap! {s("abc") => s("def")}, options: btreemap! {s("xyz") => s("foo")}, + cluster_key: Some("(a + 2, b)".to_string()), cluster_keys: vec!["(a + 2, b)".to_string()], default_cluster_key_id: Some(0), created_on: Utc.ymd(2014, 11, 28).and_hms(12, 0, 9), @@ -221,7 +222,7 @@ fn test_load_old() -> anyhow::Result<()> { // TableInfo is loadable { let tbl_info_v1: Vec = vec![ - 10, 7, 8, 5, 16, 6, 160, 6, 1, 18, 3, 102, 111, 111, 26, 3, 98, 97, 114, 34, 134, 5, + 10, 7, 8, 5, 16, 6, 160, 6, 1, 18, 3, 102, 111, 111, 26, 3, 98, 97, 114, 34, 254, 4, 10, 140, 4, 10, 37, 10, 8, 110, 117, 108, 108, 97, 98, 108, 101, 18, 5, 97, 32, 43, 32, 51, 26, 15, 10, 10, 10, 5, 26, 0, 160, 6, 1, 160, 6, 1, 160, 6, 1, 160, 6, 1, 10, 16, 10, 4, 98, 111, 111, 108, 26, 5, 18, 0, 160, 6, 1, 160, 6, 1, 10, 16, 10, 4, 105, 110, @@ -247,11 +248,11 @@ fn test_load_old() -> anyhow::Result<()> { 3, 160, 6, 1, 160, 6, 1, 160, 6, 1, 10, 26, 10, 8, 105, 110, 116, 101, 114, 118, 97, 108, 26, 11, 170, 1, 5, 8, 2, 160, 6, 1, 160, 6, 1, 160, 6, 1, 18, 6, 10, 1, 97, 18, 1, 98, 160, 6, 1, 42, 10, 10, 3, 120, 121, 122, 18, 3, 102, 111, 111, 50, 2, 52, 52, 58, - 10, 10, 3, 97, 98, 99, 18, 3, 100, 101, 102, 64, 0, 74, 10, 40, 97, 32, 43, 32, 50, 44, - 32, 98, 41, 162, 1, 23, 50, 48, 49, 52, 45, 49, 49, 45, 50, 56, 32, 49, 50, 58, 48, 48, - 58, 48, 57, 32, 85, 84, 67, 170, 1, 23, 50, 48, 49, 52, 45, 49, 49, 45, 50, 57, 32, 49, - 50, 58, 48, 48, 58, 49, 48, 32, 85, 84, 67, 178, 1, 13, 116, 97, 98, 108, 101, 95, 99, - 111, 109, 109, 101, 110, 116, 186, 1, 3, 160, 6, 1, 160, 6, 1, 160, 6, 1, + 10, 10, 3, 97, 98, 99, 18, 3, 100, 101, 102, 74, 10, 40, 97, 32, 43, 32, 50, 44, 32, + 98, 41, 162, 1, 23, 50, 48, 49, 52, 45, 49, 49, 45, 50, 56, 32, 49, 50, 58, 48, 48, 58, + 48, 57, 32, 85, 84, 67, 170, 1, 23, 50, 48, 49, 52, 45, 49, 49, 45, 50, 57, 32, 49, 50, + 58, 48, 48, 58, 49, 48, 32, 85, 84, 67, 178, 1, 13, 116, 97, 98, 108, 101, 95, 99, 111, + 109, 109, 101, 110, 116, 160, 6, 1, 160, 6, 1, ]; let p: pb::TableInfo = common_protos::prost::Message::decode(tbl_info_v1.as_slice()).map_err(print_err)?; @@ -316,8 +317,9 @@ fn test_load_old() -> anyhow::Result<()> { engine: "44".to_string(), engine_options: btreemap! {s("abc") => s("def")}, options: btreemap! {s("xyz") => s("foo")}, - cluster_keys: vec!["(a + 2, b)".to_string()], - default_cluster_key_id: Some(0), + cluster_key: Some("(a + 2, b)".to_string()), + cluster_keys: vec![], + default_cluster_key_id: None, created_on: Utc.ymd(2014, 11, 28).and_hms(12, 0, 9), updated_on: Utc.ymd(2014, 11, 29).and_hms(12, 0, 10), comment: s("table_comment"), diff --git a/common/protos/proto/metadata.proto b/common/protos/proto/metadata.proto index 9e078021b5f1..98c2c3265aaa 100644 --- a/common/protos/proto/metadata.proto +++ b/common/protos/proto/metadata.proto @@ -154,8 +154,11 @@ message TableMeta { // Table options. map options = 5; + // Keys to sort rows in table. + optional string cluster_key = 9; + // The vector of cluster keys. - repeated string cluster_keys = 9; + repeated string cluster_keys = 4; // The default cluster keys id. optional uint32 default_cluster_key_id = 8; diff --git a/query/src/interpreters/interpreter_table_show_create.rs b/query/src/interpreters/interpreter_table_show_create.rs index ea5b7961a64d..8f4fd9816d01 100644 --- a/query/src/interpreters/interpreter_table_show_create.rs +++ b/query/src/interpreters/interpreter_table_show_create.rs @@ -94,7 +94,7 @@ impl Interpreter for ShowCreateTableInterpreter { table_create_sql.push_str(table_engine.as_str()); let table_info = table.get_table_info(); - if let Some((_, cluster_keys_str)) = table_info.meta.cluster_keys() { + if let Some((_, cluster_keys_str)) = table_info.meta.cluster_key() { table_create_sql.push_str(format!(" CLUSTER BY {}", cluster_keys_str).as_str()); } diff --git a/query/src/sql/planner/binder/ddl/table.rs b/query/src/sql/planner/binder/ddl/table.rs index cab9352fa256..cd22e203eacd 100644 --- a/query/src/sql/planner/binder/ddl/table.rs +++ b/query/src/sql/planner/binder/ddl/table.rs @@ -212,8 +212,8 @@ impl<'a> Binder { cluster_keys.push(cluster_key.to_string()); } if !cluster_keys.is_empty() { - let order_keys_sql = format!("({})", cluster_keys.join(", ")); - meta.cluster_keys = Some(order_keys_sql); + let cluster_keys_sql = format!("({})", cluster_keys.join(", ")); + meta = meta.push_cluster_key(cluster_keys_sql); } let plan = CreateTablePlan { diff --git a/query/src/sql/statements/statement_create_table.rs b/query/src/sql/statements/statement_create_table.rs index f348b45d836e..f48da17705cc 100644 --- a/query/src/sql/statements/statement_create_table.rs +++ b/query/src/sql/statements/statement_create_table.rs @@ -107,7 +107,7 @@ impl AnalyzableStatement for DfCreateTable { if !cluster_keys.is_empty() { let cluster_keys: Vec = cluster_keys.iter().map(|e| e.column_name()).collect(); let cluster_keys_sql = format!("({})", cluster_keys.join(", ")); - table_meta = table_meta.set_cluster_keys_meta(cluster_keys_sql); + table_meta = table_meta.push_cluster_key(cluster_keys_sql); } Ok(AnalyzedResult::SimpleQuery(Box::new( diff --git a/query/src/storages/fuse/fuse_table.rs b/query/src/storages/fuse/fuse_table.rs index bd0d99c5aa85..604fa747bf1a 100644 --- a/query/src/storages/fuse/fuse_table.rs +++ b/query/src/storages/fuse/fuse_table.rs @@ -72,7 +72,7 @@ impl FuseTable { pub fn do_create(table_info: TableInfo, read_only: bool) -> Result> { let storage_prefix = Self::parse_storage_prefix(&table_info)?; - let cluster_key_meta = table_info.meta.cluster_keys(); + let cluster_key_meta = table_info.meta.cluster_key(); let mut cluster_keys = Vec::new(); if let Some((_, order)) = &cluster_key_meta { cluster_keys = PlanParser::parse_exprs(order)?; @@ -202,8 +202,8 @@ impl Table for FuseTable { cluster_key_str: String, ) -> Result<()> { let mut new_table_meta = self.get_table_info().meta.clone(); - new_table_meta = new_table_meta.set_cluster_keys_meta(cluster_key_str); - let cluster_key_meta = new_table_meta.cluster_keys(); + new_table_meta = new_table_meta.push_cluster_key(cluster_key_str); + let cluster_key_meta = new_table_meta.cluster_key(); let schema = self.schema().as_ref().clone(); let prev = self.read_table_snapshot(ctx.as_ref()).await?; diff --git a/query/src/storages/storage_table.rs b/query/src/storages/storage_table.rs index 0f88bf7c796f..0fcf77ae9d50 100644 --- a/query/src/storages/storage_table.rs +++ b/query/src/storages/storage_table.rs @@ -84,7 +84,7 @@ pub trait Table: Sync + Send { _cluster_key_str: String, ) -> Result<()> { Err(ErrorCode::UnsupportedEngineParams(format!( - "Unsupported cluster key for engine: {}", + "Unsupported clustering keys for engine: {}", self.engine() ))) }