chore: simplify alter and drop cluster key logic

databendlabs · Dec 27, 2024 · 8eb18e2 · 8eb18e2
1 parent cce8c74
commit 8eb18e2
Showing 51 changed files with 178 additions and 358 deletions.
diff --git a/src/common/base/src/runtime/metrics/family_metrics/counter.rs b/src/common/base/src/runtime/metrics/family_metrics/counter.rs
@@ -42,7 +42,7 @@ impl<Labels: FamilyLabels> FamilyCounter<Labels> {
         FamilyCounter {
             index,
             labels,
-            value: Arc::new(Default::default()),
+            value: Default::default(),
         }
     }
 

diff --git a/src/meta/app/src/schema/table.rs b/src/meta/app/src/schema/table.rs
@@ -262,10 +262,8 @@ pub struct TableMeta {
     pub options: BTreeMap<String, String>,
     // The default cluster key.
     pub default_cluster_key: Option<String>,
-    // All cluster keys that have been defined.
-    pub cluster_keys: Vec<String>,
-    // The sequence number of default_cluster_key in cluster_keys.
-    pub default_cluster_key_id: Option<u32>,
+    // The sequence number of default_cluster_key.
+    pub default_cluster_key_id: u32,
     pub created_on: DateTime<Utc>,
     pub updated_on: DateTime<Utc>,
     pub comment: String,
@@ -419,8 +417,7 @@ impl Default for TableMeta {
             part_prefix: "".to_string(),
             options: BTreeMap::new(),
             default_cluster_key: None,
-            cluster_keys: vec![],
-            default_cluster_key_id: None,
+            default_cluster_key_id: 0,
             created_on: Utc::now(),
             updated_on: Utc::now(),
             comment: "".to_string(),
@@ -434,20 +431,6 @@ impl Default for TableMeta {
     }
 }
 
-impl TableMeta {
-    pub fn push_cluster_key(mut self, cluster_key: String) -> Self {
-        self.cluster_keys.push(cluster_key.clone());
-        self.default_cluster_key = Some(cluster_key);
-        self.default_cluster_key_id = Some(self.cluster_keys.len() as u32 - 1);
-        self
-    }
-
-    pub fn cluster_key(&self) -> Option<(u32, String)> {
-        self.default_cluster_key_id
-            .zip(self.default_cluster_key.clone())
-    }
-}
-
 impl Display for TableMeta {
     fn fmt(&self, f: &mut Formatter) -> fmt::Result {
         write!(

diff --git a/src/meta/proto-conv/src/table_from_to_protobuf_impl.rs b/src/meta/proto-conv/src/table_from_to_protobuf_impl.rs
@@ -192,6 +192,12 @@ impl FromToProto for mt::TableMeta {
             indexes.insert(name, mt::TableIndex::from_pb(index)?);
         }
 
+        let default_cluster_key_id = if let Some(cluster_key_id) = p.default_cluster_key_id {
+            cluster_key_id
+        } else {
+            p.cluster_keys.len() as u32 - 1
+        };
+
         let v = Self {
             schema: Arc::new(ex::TableSchema::from_pb(schema)?),
             engine: p.engine,
@@ -203,8 +209,7 @@ impl FromToProto for mt::TableMeta {
             part_prefix: p.part_prefix.unwrap_or("".to_string()),
             options: p.options,
             default_cluster_key: p.default_cluster_key,
-            cluster_keys: p.cluster_keys,
-            default_cluster_key_id: p.default_cluster_key_id,
+            default_cluster_key_id,
             created_on: DateTime::<Utc>::from_pb(p.created_on)?,
             updated_on: DateTime::<Utc>::from_pb(p.updated_on)?,
             drop_on: match p.drop_on {
@@ -251,8 +256,9 @@ impl FromToProto for mt::TableMeta {
             },
             options: self.options.clone(),
             default_cluster_key: self.default_cluster_key.clone(),
-            cluster_keys: self.cluster_keys.clone(),
-            default_cluster_key_id: self.default_cluster_key_id,
+            // cluster_keys is deprecated.
+            cluster_keys: vec![],
+            default_cluster_key_id: Some(self.default_cluster_key_id),
             created_on: self.created_on.to_pb()?,
             updated_on: self.updated_on.to_pb()?,
             drop_on: match self.drop_on {

diff --git a/src/meta/proto-conv/tests/it/proto_conv.rs b/src/meta/proto-conv/tests/it/proto_conv.rs
@@ -141,8 +141,7 @@ fn new_table_meta() -> mt::TableMeta {
         engine_options: btreemap! {s("abc") => s("def")},
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/meta/proto-conv/tests/it/v002_table_meta.rs b/src/meta/proto-conv/tests/it/v002_table_meta.rs
@@ -132,8 +132,7 @@ fn test_decode_v2_table_meta() -> anyhow::Result<()> {
         engine_options: btreemap! {s("abc") => s("def")},
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/meta/proto-conv/tests/it/v010_table_meta.rs b/src/meta/proto-conv/tests/it/v010_table_meta.rs
@@ -134,8 +134,7 @@ fn test_decode_v10_table_meta() -> anyhow::Result<()> {
         part_prefix: "".to_string(),
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/meta/proto-conv/tests/it/v012_table_meta.rs b/src/meta/proto-conv/tests/it/v012_table_meta.rs
@@ -136,8 +136,7 @@ fn test_decode_v12_table_meta() -> anyhow::Result<()> {
         part_prefix: "".to_string(),
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/meta/proto-conv/tests/it/v023_table_meta.rs b/src/meta/proto-conv/tests/it/v023_table_meta.rs
@@ -136,8 +136,7 @@ fn test_decode_v23_table_meta() -> anyhow::Result<()> {
         part_prefix: "lulu_".to_string(),
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/meta/proto-conv/tests/it/v024_table_meta.rs b/src/meta/proto-conv/tests/it/v024_table_meta.rs
@@ -136,8 +136,7 @@ fn test_decode_v24_table_meta() -> anyhow::Result<()> {
         part_prefix: "lulu_".to_string(),
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/meta/proto-conv/tests/it/v033_table_meta.rs b/src/meta/proto-conv/tests/it/v033_table_meta.rs
@@ -138,8 +138,7 @@ fn test_decode_v33_table_meta() -> anyhow::Result<()> {
         engine_options: btreemap! {s("abc") => s("def")},
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/meta/proto-conv/tests/it/v040_table_meta.rs b/src/meta/proto-conv/tests/it/v040_table_meta.rs
@@ -138,8 +138,7 @@ fn test_decode_v40_table_meta() -> anyhow::Result<()> {
         engine_options: btreemap! {s("abc") => s("def")},
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/meta/proto-conv/tests/it/v044_table_meta.rs b/src/meta/proto-conv/tests/it/v044_table_meta.rs
@@ -89,8 +89,7 @@ fn test_decode_v44_table_meta() -> anyhow::Result<()> {
         engine_options: btreemap! {s("abc") => s("def")},
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/meta/proto-conv/tests/it/v055_table_meta.rs b/src/meta/proto-conv/tests/it/v055_table_meta.rs
@@ -80,8 +80,7 @@ fn test_decode_v55_table_meta() -> anyhow::Result<()> {
         engine_options: btreemap! {s("abc") => s("def")},
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/meta/proto-conv/tests/it/v074_table_db_meta.rs b/src/meta/proto-conv/tests/it/v074_table_db_meta.rs
@@ -79,8 +79,7 @@ fn test_decode_v74_table_meta() -> anyhow::Result<()> {
         engine_options: btreemap! {s("abc") => s("def")},
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/meta/proto-conv/tests/it/v080_geometry_datatype.rs b/src/meta/proto-conv/tests/it/v080_geometry_datatype.rs
@@ -206,8 +206,7 @@ fn test_decode_v80_table_meta() -> anyhow::Result<()> {
         engine_options: btreemap! {s("abc") => s("def")},
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/meta/proto-conv/tests/it/v082_table_index.rs b/src/meta/proto-conv/tests/it/v082_table_index.rs
@@ -80,8 +80,7 @@ fn test_decode_v82_table_meta() -> anyhow::Result<()> {
         engine_options: btreemap! {s("abc") => s("def")},
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/meta/proto-conv/tests/it/v085_table_index.rs b/src/meta/proto-conv/tests/it/v085_table_index.rs
@@ -80,8 +80,7 @@ fn test_decode_v85_table_meta() -> anyhow::Result<()> {
         engine_options: btreemap! {s("abc") => s("def")},
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/meta/proto-conv/tests/it/v086_table_index.rs b/src/meta/proto-conv/tests/it/v086_table_index.rs
@@ -83,8 +83,7 @@ fn test_decode_v86_table_meta() -> anyhow::Result<()> {
         engine_options: btreemap! {s("abc") => s("def")},
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/meta/proto-conv/tests/it/v094_table_meta.rs b/src/meta/proto-conv/tests/it/v094_table_meta.rs
@@ -78,8 +78,7 @@ fn test_decode_v94_table_meta() -> anyhow::Result<()> {
         engine_options: btreemap! {s("abc") => s("def")},
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/meta/proto-conv/tests/it/v107_geography_datatype.rs b/src/meta/proto-conv/tests/it/v107_geography_datatype.rs
@@ -211,8 +211,7 @@ fn test_decode_v107_table_meta() -> anyhow::Result<()> {
         engine_options: btreemap! {s("abc") => s("def")},
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/meta/proto-conv/tests/it/v114_interval_datatype.rs b/src/meta/proto-conv/tests/it/v114_interval_datatype.rs
@@ -202,8 +202,7 @@ fn test_decode_v114_table_meta() -> anyhow::Result<()> {
         engine_options: btreemap! {s("abc") => s("def")},
         options: btreemap! {s("xyz") => s("foo")},
         default_cluster_key: Some("(a + 2, b)".to_string()),
-        cluster_keys: vec!["(a + 2, b)".to_string()],
-        default_cluster_key_id: Some(0),
+        default_cluster_key_id: 0,
         created_on: Utc.with_ymd_and_hms(2014, 11, 28, 12, 0, 9).unwrap(),
         updated_on: Utc.with_ymd_and_hms(2014, 11, 29, 12, 0, 10).unwrap(),
         comment: s("table_comment"),

diff --git a/src/query/catalog/src/table.rs b/src/query/catalog/src/table.rs
@@ -14,6 +14,7 @@
 
 use std::any::Any;
 use std::collections::BTreeMap;
+use std::collections::HashMap;
 use std::sync::Arc;
 
 use chrono::DateTime;
@@ -22,12 +23,13 @@ use databend_common_exception::ErrorCode;
 use databend_common_exception::Result;
 use databend_common_expression::BlockThresholds;
 use databend_common_expression::ColumnId;
-use databend_common_expression::RemoteExpr;
 use databend_common_expression::Scalar;
 use databend_common_expression::TableSchema;
 use databend_common_io::constants::DEFAULT_BLOCK_BUFFER_SIZE;
 use databend_common_io::constants::DEFAULT_BLOCK_MAX_ROWS;
 use databend_common_io::constants::DEFAULT_BLOCK_MIN_ROWS;
+use databend_common_meta_app::app_error::AppError;
+use databend_common_meta_app::app_error::UnknownTableId;
 use databend_common_meta_app::schema::TableIdent;
 use databend_common_meta_app::schema::TableInfo;
 use databend_common_meta_app::schema::TableMeta;
@@ -37,9 +39,12 @@ use databend_common_meta_types::MetaId;
 use databend_common_pipeline_core::Pipeline;
 use databend_common_storage::Histogram;
 use databend_common_storage::StorageMetrics;
+use databend_storages_common_table_meta::meta::ClusterKey;
 use databend_storages_common_table_meta::meta::SnapshotId;
 use databend_storages_common_table_meta::meta::TableSnapshot;
 use databend_storages_common_table_meta::table::ChangeType;
+use databend_storages_common_table_meta::table::ClusterType;
+use databend_storages_common_table_meta::table::OPT_KEY_CLUSTER_TYPE;
 use databend_storages_common_table_meta::table::OPT_KEY_TEMP_PREFIX;
 use databend_storages_common_table_meta::table_id_ranges::is_temp_table_id;
 
@@ -117,8 +122,18 @@ pub trait Table: Sync + Send {
         false
     }
 
-    fn cluster_keys(&self, _ctx: Arc<dyn TableContext>) -> Vec<RemoteExpr<String>> {
-        vec![]
+    fn cluster_key_meta(&self) -> Option<ClusterKey> {
+        None
+    }
+
+    fn cluster_type(&self) -> Option<ClusterType> {
+        self.cluster_key_meta()?;
+        let cluster_type = self
+            .options()
+            .get(OPT_KEY_CLUSTER_TYPE)
+            .and_then(|s| s.parse::<ClusterType>().ok())
+            .unwrap_or(ClusterType::Linear);
+        Some(cluster_type)
     }
 
     fn change_tracking_enabled(&self) -> bool {
@@ -159,31 +174,6 @@ pub trait Table: Sync + Send {
         false
     }
 
-    #[async_backtrace::framed]
-    async fn alter_table_cluster_keys(
-        &self,
-        ctx: Arc<dyn TableContext>,
-        cluster_key: String,
-        cluster_type: String,
-    ) -> Result<()> {
-        let (_, _, _) = (ctx, cluster_key, cluster_type);
-
-        Err(ErrorCode::UnsupportedEngineParams(format!(
-            "Altering table cluster keys is not supported for the '{}' engine.",
-            self.engine()
-        )))
-    }
-
-    #[async_backtrace::framed]
-    async fn drop_table_cluster_keys(&self, ctx: Arc<dyn TableContext>) -> Result<()> {
-        let _ = ctx;
-
-        Err(ErrorCode::UnsupportedEngineParams(format!(
-            "Dropping table cluster keys is not supported for the '{}' engine.",
-            self.engine()
-        )))
-    }
-
     /// Gather partitions to be scanned according to the push_downs
     #[async_backtrace::framed]
     async fn read_partitions(
@@ -584,11 +574,6 @@ pub struct NavigationDescriptor {
     pub point: NavigationPoint,
 }
 
-use std::collections::HashMap;
-
-use databend_common_meta_app::app_error::AppError;
-use databend_common_meta_app::app_error::UnknownTableId;
-
 #[derive(serde::Serialize, serde::Deserialize, Clone, Debug, Default)]
 pub struct ParquetTableColumnStatisticsProvider {
     column_stats: HashMap<ColumnId, Option<BasicColumnStatistics>>,

diff --git a/src/query/service/src/interpreters/hook/compact_hook.rs b/src/query/service/src/interpreters/hook/compact_hook.rs
@@ -194,7 +194,7 @@ async fn compact_table(
 
     {
         // do recluster.
-        if !table.cluster_keys(ctx.clone()).is_empty() {
+        if table.cluster_key_meta().is_some() {
             let recluster = RelOperator::Recluster(Recluster {
                 catalog: compact_target.catalog,
                 database: compact_target.database,