Skip to content

Commit

Permalink
fix review comment
Browse files Browse the repository at this point in the history
  • Loading branch information
zhyass committed Jun 5, 2022
1 parent 2ac7925 commit abe3d38
Show file tree
Hide file tree
Showing 8 changed files with 29 additions and 20 deletions.
12 changes: 7 additions & 5 deletions common/meta/app/src/schema/table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ pub struct TableMeta {
pub engine: String,
pub engine_options: BTreeMap<String, String>,
pub options: BTreeMap<String, String>,
pub cluster_key: Option<String>,
// The vector of cluster keys.
pub cluster_keys: Vec<String>,
// The default cluster keys id.
Expand Down Expand Up @@ -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(),
Expand All @@ -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())
}
}

Expand Down
2 changes: 2 additions & 0 deletions common/proto-conv/src/table_from_to_protobuf_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ impl FromToProto<pb::TableMeta> 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::<Utc>::from_pb(p.created_on)?,
Expand All @@ -151,6 +152,7 @@ impl FromToProto<pb::TableMeta> 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()?,
Expand Down
18 changes: 10 additions & 8 deletions common/proto-conv/tests/it/proto_conv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -221,7 +222,7 @@ fn test_load_old() -> anyhow::Result<()> {
// TableInfo is loadable
{
let tbl_info_v1: Vec<u8> = 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,
Expand All @@ -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)?;
Expand Down Expand Up @@ -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"),
Expand Down
5 changes: 4 additions & 1 deletion common/protos/proto/metadata.proto
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,11 @@ message TableMeta {
// Table options.
map<string, string> 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;
Expand Down
2 changes: 1 addition & 1 deletion query/src/interpreters/interpreter_table_show_create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down
2 changes: 1 addition & 1 deletion query/src/sql/statements/statement_create_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ impl AnalyzableStatement for DfCreateTable {
if !cluster_keys.is_empty() {
let cluster_keys: Vec<String> = 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(
Expand Down
6 changes: 3 additions & 3 deletions query/src/storages/fuse/fuse_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ impl FuseTable {

pub fn do_create(table_info: TableInfo, read_only: bool) -> Result<Box<FuseTable>> {
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)?;
Expand Down Expand Up @@ -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?;
Expand Down
2 changes: 1 addition & 1 deletion query/src/storages/storage_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
)))
}
Expand Down

0 comments on commit abe3d38

Please sign in to comment.