fix: alter column with specify approx distinct (#18928)

* fix: alter column with approx distinct

* fix

* fix

Author: zhyass

src/query/service/src/interpreters/interpreter_table_add_column.rs

@@ -134,6 +134,12 @@ impl Interpreter for AddTableColumnInterpreter {
         // need rebuild the table to generate stored computed column.
         if num_rows > 0 && matches!(field.computed_expr, Some(ComputedExpr::Stored(_))) {
             let fuse_table = FuseTable::try_from_table(tbl.as_ref())?;
+            if fuse_table.change_tracking_enabled() {
+                return Err(ErrorCode::AlterTableError(format!(
+                    "Cannot add stored computed column to table '{}' with change tracking enabled",
+                    table_info.desc
+                )));
+            }
             let base_snapshot = fuse_table.read_table_snapshot().await?;
             let prev_snapshot_id = base_snapshot.snapshot_id().map(|(id, _)| id);
             let table_meta_timestamps = self
@@ -165,6 +171,16 @@ impl Interpreter for AddTableColumnInterpreter {
             .await;
         }
 
+        let need_update = num_rows > 0 && !self.plan.is_deterministic;
+        if need_update && tbl.change_tracking_enabled() {
+            // Rebuild table while change tracking is active may break the consistency
+            // of tracked changes, leading to incorrect change records.
+            return Err(ErrorCode::AlterTableError(format!(
+                "Cannot add non-deterministic default column to table '{}' with change tracking enabled",
+                table_info.desc
+            )));
+        }
+
         let new_table_meta = table_info.meta.clone();
         commit_table_meta(
             &self.ctx,
@@ -177,7 +193,7 @@ impl Interpreter for AddTableColumnInterpreter {
 
         // If the column is not deterministic and table is non-empty,
         // update to refresh the value with default expr.
-        if num_rows > 0 && !self.plan.is_deterministic {
+        if need_update {
             self.ctx
                 .evict_table_from_cache(catalog_name, db_name, tbl_name)?;
             let query = format!(

src/query/service/src/interpreters/interpreter_table_drop_column.rs

@@ -20,9 +20,11 @@ use databend_common_exception::Result;
 use databend_common_expression::DataSchema;
 use databend_common_meta_app::schema::DatabaseType;
 use databend_common_sql::plans::DropTableColumnPlan;
+use databend_common_sql::ApproxDistinctColumns;
 use databend_common_sql::BloomIndexColumns;
 use databend_common_storages_basic::view_table::VIEW_ENGINE;
 use databend_common_storages_stream::stream_table::STREAM_ENGINE;
+use databend_storages_common_table_meta::table::OPT_KEY_APPROX_DISTINCT_COLUMNS;
 use databend_storages_common_table_meta::table::OPT_KEY_BLOOM_INDEX_COLUMNS;
 
 use crate::interpreters::common::check_referenced_computed_columns;
@@ -130,6 +132,17 @@ impl Interpreter for DropTableColumnInterpreter {
                 }
             }
         }
+        if let Some(value) = opts.get_mut(OPT_KEY_APPROX_DISTINCT_COLUMNS) {
+            if let ApproxDistinctColumns::Specify(mut cols) =
+                value.parse::<ApproxDistinctColumns>()?
+            {
+                if let Some(pos) = cols.iter().position(|x| *x == self.plan.column) {
+                    // remove from the approx distinct columns.
+                    cols.remove(pos);
+                    *value = cols.join(",");
+                }
+            }
+        }
 
         commit_table_meta(
             &self.ctx,

src/query/service/src/interpreters/interpreter_table_modify_column.rs

@@ -44,6 +44,7 @@ use databend_common_sql::plans::ModifyColumnAction;
 use databend_common_sql::plans::ModifyTableColumnPlan;
 use databend_common_sql::plans::Plan;
 use databend_common_sql::resolve_type_name_by_str;
+use databend_common_sql::ApproxDistinctColumns;
 use databend_common_sql::BloomIndexColumns;
 use databend_common_sql::DefaultExprBinder;
 use databend_common_sql::Planner;
@@ -53,9 +54,11 @@ use databend_common_storages_stream::stream_table::STREAM_ENGINE;
 use databend_common_users::UserApiProvider;
 use databend_enterprise_data_mask_feature::get_datamask_handler;
 use databend_storages_common_index::BloomIndex;
+use databend_storages_common_index::RangeIndex;
 use databend_storages_common_table_meta::meta::SnapshotId;
 use databend_storages_common_table_meta::meta::TableMetaTimestamps;
 use databend_storages_common_table_meta::readers::snapshot_reader::TableSnapshotAccessor;
+use databend_storages_common_table_meta::table::OPT_KEY_APPROX_DISTINCT_COLUMNS;
 use databend_storages_common_table_meta::table::OPT_KEY_BLOOM_INDEX_COLUMNS;
 
 use crate::interpreters::common::check_referenced_computed_columns;
@@ -246,6 +249,13 @@ impl ModifyTableColumnInterpreter {
             }
         }
 
+        let mut approx_distinct_cols = vec![];
+        if let Some(v) = table_info.options().get(OPT_KEY_APPROX_DISTINCT_COLUMNS) {
+            if let ApproxDistinctColumns::Specify(cols) = v.parse::<ApproxDistinctColumns>()? {
+                approx_distinct_cols = cols;
+            }
+        }
+
         let mut table_info = table.get_table_info().clone();
         table_info.meta.fill_field_comments();
         let mut modify_comment = false;
@@ -272,6 +282,16 @@ impl ModifyTableColumnInterpreter {
                             field.data_type
                         )));
                     }
+                    if approx_distinct_cols
+                        .iter()
+                        .any(|v| v.as_str() == field.name)
+                        && !RangeIndex::supported_table_type(&field.data_type)
+                    {
+                        return Err(ErrorCode::TableOptionInvalid(format!(
+                            "Unsupported data type '{}' for approx distinct columns",
+                            field.data_type
+                        )));
+                    }
                     // If the column is inverted index column, the type can't be changed.
                     if !table_info.meta.indexes.is_empty() {
                         for (index_name, index) in &table_info.meta.indexes {

tests/sqllogictests/suites/base/05_ddl/05_0003_ddl_alter_table.test

@@ -118,6 +118,31 @@ ALTER TABLE `05_0003_at_t3` MODIFY COLUMN c float not null
 statement ok
 DROP TABLE IF EXISTS `05_0003_at_t3`
 
+statement ok
+CREATE OR REPLACE TABLE `05_0003_at_t6` ( a int, b string, c int) approx_distinct_columns = 'a,b' COMPRESSION='zstd' STORAGE_FORMAT='native'
+
+statement ok
+ALTER TABLE `05_0003_at_t6` drop column a;
+
+query TT
+SHOW CREATE TABLE `05_0003_at_t6`;
+----
+05_0003_at_t6 CREATE TABLE "05_0003_at_t6" ( b VARCHAR NULL, c INT NULL ) ENGINE=FUSE APPROX_DISTINCT_COLUMNS='b' COMPRESSION='zstd' ENABLE_AUTO_ANALYZE='1' STORAGE_FORMAT='native'
+
+statement error 1301
+ALTER TABLE `05_0003_at_t6` MODIFY COLUMN b binary; 
+
+statement ok
+ALTER TABLE `05_0003_at_t6` drop column b;
+
+query TT
+SHOW CREATE TABLE `05_0003_at_t6`;
+----
+05_0003_at_t6 CREATE TABLE "05_0003_at_t6" ( c INT NULL ) ENGINE=FUSE APPROX_DISTINCT_COLUMNS='' COMPRESSION='zstd' ENABLE_AUTO_ANALYZE='1' STORAGE_FORMAT='native'
+
+statement ok
+DROP TABLE IF EXISTS `05_0003_at_t6`
+
 statement ok
 set hide_options_in_show_create_table=1
 

tests/sqllogictests/suites/ee/06_ee_stream/06_0009_stream_issue_18827.test

@@ -32,6 +32,9 @@ alter table t_18827 modify column b float64;
 statement ok
 alter table t_18827 modify column a binary;
 
+statement error 1132
+alter table t_18827 add column c date default today();
+
 query T
 select a, b from t_18827;
 ----