fix: alter table modify type should also modify default value (#6049)

* fix: select after alter

* fix: insert a proper row&catch a bug

* fix: alter table modify type modify default value type too

* refactor: per review

* chore: per review

* refactor: per review

* refactor: per review

src/datatypes/src/schema/constraint.rs

@@ -20,6 +20,7 @@ use snafu::{ensure, ResultExt};
 
 use crate::data_type::{ConcreteDataType, DataType};
 use crate::error::{self, Result};
+use crate::types::cast;
 use crate::value::Value;
 use crate::vectors::operations::VectorOp;
 use crate::vectors::{TimestampMillisecondVector, VectorRef};
@@ -178,6 +179,18 @@ impl ColumnDefaultConstraint {
         }
     }
 
+    /// Cast default value to given type
+    pub fn cast_to_datatype(&self, data_type: &ConcreteDataType) -> Result<Self> {
+        match self {
+            ColumnDefaultConstraint::Value(v) => Ok(Self::Value(cast(v.clone(), data_type)?)),
+            ColumnDefaultConstraint::Function(expr) => match &expr[..] {
+                // no need to cast, since function always require a data_type when need to create default value
+                CURRENT_TIMESTAMP | CURRENT_TIMESTAMP_FN | NOW_FN => Ok(self.clone()),
+                _ => error::UnsupportedDefaultExprSnafu { expr }.fail(),
+            },
+        }
+    }
+
     /// Only create default vector if it's impure, i.e., it's a function.
     ///
     /// This helps to delay creating constant default values to mito engine while also keeps impure default have consistent values

src/mito2/src/error.rs

@@ -243,6 +243,8 @@ pub enum Error {
         region_id: RegionId,
         column: String,
         source: datatypes::Error,
+        #[snafu(implicit)]
+        location: Location,
     },
 
     #[snafu(display("Failed to build entry, region_id: {}", region_id))]

src/store-api/src/metadata.rs

@@ -567,7 +567,7 @@ impl RegionMetadataBuilder {
         match kind {
             AlterKind::AddColumns { columns } => self.add_columns(columns)?,
             AlterKind::DropColumns { names } => self.drop_columns(&names),
-            AlterKind::ModifyColumnTypes { columns } => self.modify_column_types(columns),
+            AlterKind::ModifyColumnTypes { columns } => self.modify_column_types(columns)?,
             AlterKind::SetIndex { options } => match options {
                 ApiSetIndexOptions::Fulltext {
                     column_name,
@@ -681,7 +681,7 @@ impl RegionMetadataBuilder {
     }
 
     /// Changes columns type to the metadata if exist.
-    fn modify_column_types(&mut self, columns: Vec<ModifyColumnType>) {
+    fn modify_column_types(&mut self, columns: Vec<ModifyColumnType>) -> Result<()> {
         let mut change_type_map: HashMap<_, _> = columns
             .into_iter()
             .map(
@@ -694,9 +694,34 @@ impl RegionMetadataBuilder {
 
         for column_meta in self.column_metadatas.iter_mut() {
             if let Some(target_type) = change_type_map.remove(&column_meta.column_schema.name) {
-                column_meta.column_schema.data_type = target_type;
+                column_meta.column_schema.data_type = target_type.clone();
+                // also cast default value to target_type if default value exist
+                let new_default =
+                    if let Some(default_value) = column_meta.column_schema.default_constraint() {
+                        Some(
+                            default_value
+                                .cast_to_datatype(&target_type)
+                                .with_context(|_| CastDefaultValueSnafu {
+                                    reason: format!(
+                                        "Failed to cast default value from {:?} to type {:?}",
+                                        default_value, target_type
+                                    ),
+                                })?,
+                        )
+                    } else {
+                        None
+                    };
+                column_meta.column_schema = column_meta
+                    .column_schema
+                    .clone()
+                    .with_default_constraint(new_default.clone())
+                    .with_context(|_| CastDefaultValueSnafu {
+                        reason: format!("Failed to set new default: {:?}", new_default),
+                    })?;
             }
         }
+
+        Ok(())
     }
 
     fn change_column_inverted_index_options(
@@ -967,6 +992,14 @@ pub enum MetadataError {
         location: Location,
     },
 
+    #[snafu(display("Failed to cast default value, reason: {}", reason))]
+    CastDefaultValue {
+        reason: String,
+        source: datatypes::Error,
+        #[snafu(implicit)]
+        location: Location,
+    },
+
     #[snafu(display("Unexpected: {}", reason))]
     Unexpected {
         reason: String,

tests-fuzz/src/validator/column.rs

@@ -17,7 +17,7 @@ use datatypes::data_type::DataType;
 use snafu::{ensure, ResultExt};
 use sqlx::MySqlPool;
 
-use crate::error::{self, Result};
+use crate::error::{self, Result, UnexpectedSnafu};
 use crate::ir::create_expr::ColumnOption;
 use crate::ir::{Column, Ident};
 
@@ -211,6 +211,35 @@ pub async fn fetch_columns(
         .context(error::ExecuteQuerySnafu { sql })
 }
 
+/// validate that apart from marker column(if it still exists, every thing else is just default value)
+#[allow(unused)]
+pub async fn valid_marker_value(
+    db: &MySqlPool,
+    _schema_name: Ident,
+    table_name: Ident,
+) -> Result<bool> {
+    let sql = format!("SELECT * FROM {table_name}");
+    // cache is useless and buggy anyway since alter happens all the time
+    // TODO(discord9): invalid prepared sql after alter
+    let res = sqlx::query(&sql)
+        .persistent(false)
+        .fetch_all(db)
+        .await
+        .context(error::ExecuteQuerySnafu { sql })?;
+    if res.len() != 1 {
+        UnexpectedSnafu {
+            violated: format!(
+                "Expected one row after alter table, found {}:{:?}",
+                res.len(),
+                res
+            ),
+        }
+        .fail()?
+    }
+    // TODO(discord9): make sure marker value is set
+    Ok(true)
+}
+
 #[cfg(test)]
 mod tests {
     use datatypes::data_type::{ConcreteDataType, DataType};

tests-fuzz/targets/ddl/fuzz_alter_table.rs

@@ -19,6 +19,7 @@ use std::sync::Arc;
 
 use arbitrary::{Arbitrary, Unstructured};
 use common_telemetry::info;
+use datatypes::prelude::{ConcreteDataType, DataType};
 use libfuzzer_sys::fuzz_target;
 use rand::{Rng, SeedableRng};
 use rand_chacha::ChaChaRng;
@@ -26,7 +27,7 @@ use snafu::ResultExt;
 use sqlx::{MySql, Pool};
 use strum::{EnumIter, IntoEnumIterator};
 use tests_fuzz::context::{TableContext, TableContextRef};
-use tests_fuzz::error::{self, Result};
+use tests_fuzz::error::{self, Result, UnexpectedSnafu};
 use tests_fuzz::fake::{
     merge_two_word_map_fn, random_capitalize_map, uppercase_and_keyword_backtick_map,
     MappedGenerator, WordGenerator,
@@ -94,6 +95,63 @@ fn generate_create_table_expr<R: Rng + 'static>(rng: &mut R) -> Result<CreateTab
     create_table_generator.generate(rng)
 }
 
+/// Returns the insert sql that insert one row of non-default value in table
+#[allow(unused)]
+fn generate_insert_table_sql<R: Rng + 'static>(
+    rng: &mut R,
+    create_table_expr: &CreateTableExpr,
+) -> Result<String> {
+    use datatypes::value::Value;
+    let mut column_names = vec![];
+    let mut column_values = vec![];
+    for column in &create_table_expr.columns {
+        if column.is_time_index() {
+            continue;
+        }
+
+        let marker_name = &column.name;
+        let marker_value = match column.column_type {
+            ConcreteDataType::Boolean(_) => Value::Boolean(true),
+            ConcreteDataType::Int16(_) => Value::Int16(42),
+            ConcreteDataType::Int32(_) => Value::Int32(42),
+            ConcreteDataType::Int64(_) => Value::Int64(42),
+            ConcreteDataType::Float32(_) => Value::from(42.0f32),
+            ConcreteDataType::Float64(_) => Value::from(42.0f64),
+            ConcreteDataType::String(_) => Value::from("How many roads must a man walk down?"),
+            _ => UnexpectedSnafu {
+                violated: format!(
+                    "Unexpected datatype, found {} in create-table={:?}",
+                    column.column_type, create_table_expr
+                ),
+            }
+            .fail()?,
+        };
+        column_names.push(marker_name.to_string());
+        column_values.push(marker_value.to_string());
+    }
+
+    let table_name = &create_table_expr.table_name;
+
+    let ts_column = create_table_expr
+        .columns
+        .iter()
+        .find(|c| c.is_time_index())
+        .unwrap();
+
+    let ts_column_name = &ts_column.name;
+    let ts_column_value = ts_column.column_type.default_value();
+
+    let concat_column_names = column_names.join(",");
+
+    let concat_column_values = column_values.join(",");
+
+    let sql = format!(
+        "INSERT INTO {table_name} ({concat_column_names}, {ts_column_name}) VALUES ({concat_column_values}, \"{ts_column_value}\");"
+    );
+
+    Ok(sql)
+}
+
 fn generate_alter_table_expr<R: Rng + 'static>(
     table_ctx: TableContextRef,
     rng: &mut R,
@@ -173,6 +231,13 @@ async fn execute_alter_table(ctx: FuzzContext, input: FuzzInput) -> Result<()> {
         .context(error::ExecuteQuerySnafu { sql: &sql })?;
     info!("Create table: {sql}, result: {result:?}");
 
+    let insert = generate_insert_table_sql(&mut rng, &expr)?;
+    let result = sqlx::query(&insert)
+        .execute(&ctx.greptime)
+        .await
+        .context(error::ExecuteQuerySnafu { sql: &insert })?;
+    info!("Insert Into table: {insert}, result: {result:?}");
+
     // Alter table actions
     let mut table_ctx = Arc::new(TableContext::from(&expr));
     for _ in 0..input.actions {
@@ -187,6 +252,14 @@ async fn execute_alter_table(ctx: FuzzContext, input: FuzzInput) -> Result<()> {
         // Applies changes
         table_ctx = Arc::new(Arc::unwrap_or_clone(table_ctx).alter(expr).unwrap());
 
+        // validate value
+        validator::column::valid_marker_value(
+            &ctx.greptime,
+            "public".into(),
+            table_ctx.name.clone(),
+        )
+        .await?;
+
         // Validates columns
         let mut column_entries = validator::column::fetch_columns(
             &ctx.greptime,

tests/cases/standalone/common/alter/alter_table.result

@@ -69,6 +69,52 @@ ALTER TABLE test_alt_table MODIFY COLUMN m interval;
 
 Error: 1001(Unsupported), Not supported: Modify column type to interval type
 
+INSERT INTO test_alt_table (h, i, j, m, dt) VALUES (42, 42, 0, 11, 0);
+
+Affected Rows: 1
+
+ALTER TABLE test_alt_table MODIFY COLUMN m Float64;
+
+Affected Rows: 0
+
+SELECT * FROM test_alt_table;
+
++----+----+-------------------------+---+------+---------------------+
+| h  | i  | j                       | k | m    | dt                  |
++----+----+-------------------------+---+------+---------------------+
+| 1  | 1  | 1970-01-01T00:00:00     |   |      |                     |
+| 2  | 2  | 1970-01-01T00:00:00.001 |   |      |                     |
+| 42 | 42 | 1970-01-01T00:00:00     |   | 11.0 | 1970-01-01T00:00:00 |
++----+----+-------------------------+---+------+---------------------+
+
+ALTER TABLE test_alt_table MODIFY COLUMN m INTEGER;
+
+Affected Rows: 0
+
+SELECT * FROM test_alt_table;
+
++----+----+-------------------------+---+----+---------------------+
+| h  | i  | j                       | k | m  | dt                  |
++----+----+-------------------------+---+----+---------------------+
+| 1  | 1  | 1970-01-01T00:00:00     |   |    |                     |
+| 2  | 2  | 1970-01-01T00:00:00.001 |   |    |                     |
+| 42 | 42 | 1970-01-01T00:00:00     |   | 11 | 1970-01-01T00:00:00 |
++----+----+-------------------------+---+----+---------------------+
+
+ALTER TABLE test_alt_table MODIFY COLUMN m BOOLEAN;
+
+Affected Rows: 0
+
+SELECT * FROM test_alt_table;
+
++----+----+-------------------------+---+------+---------------------+
+| h  | i  | j                       | k | m    | dt                  |
++----+----+-------------------------+---+------+---------------------+
+| 1  | 1  | 1970-01-01T00:00:00     |   |      |                     |
+| 2  | 2  | 1970-01-01T00:00:00.001 |   |      |                     |
+| 42 | 42 | 1970-01-01T00:00:00     |   | true | 1970-01-01T00:00:00 |
++----+----+-------------------------+---+------+---------------------+
+
 DESC TABLE test_alt_table;
 
 +--------+----------------------+-----+------+---------+---------------+
@@ -78,7 +124,7 @@ DESC TABLE test_alt_table;
 | i      | Int32                | PRI | YES  |         | TAG           |
 | j      | TimestampMillisecond | PRI | NO   |         | TIMESTAMP     |
 | k      | String               | PRI | YES  |         | TAG           |
-| m      | Int32                |     | YES  |         | FIELD         |
+| m      | Boolean              |     | YES  |         | FIELD         |
 | dt     | TimestampMicrosecond |     | YES  |         | FIELD         |
 +--------+----------------------+-----+------+---------+---------------+
 
@@ -86,6 +132,86 @@ DROP TABLE test_alt_table;
 
 Affected Rows: 0
 
+-- test if column with default value can change type properly
+CREATE TABLE test_alt_table_default(h INTEGER, i INTEGER DEFAULT 0, j TIMESTAMP TIME INDEX, PRIMARY KEY (h));
+
+Affected Rows: 0
+
+ALTER TABLE test_alt_table_default MODIFY COLUMN i BOOLEAN;
+
+Affected Rows: 0
+
+INSERT INTO test_alt_table_default (h, j) VALUES (1, 0), (2, 1);
+
+Affected Rows: 2
+
+SELECT * FROM test_alt_table_default ORDER BY h;
+
++---+-------+-------------------------+
+| h | i     | j                       |
++---+-------+-------------------------+
+| 1 | false | 1970-01-01T00:00:00     |
+| 2 | false | 1970-01-01T00:00:00.001 |
++---+-------+-------------------------+
+
+ALTER TABLE test_alt_table_default MODIFY COLUMN i INTEGER;
+
+Affected Rows: 0
+
+DESC TABLE test_alt_table_default;
+
++--------+----------------------+-----+------+---------+---------------+
+| Column | Type                 | Key | Null | Default | Semantic Type |
++--------+----------------------+-----+------+---------+---------------+
+| h      | Int32                | PRI | YES  |         | TAG           |
+| i      | Int32                |     | YES  | 0       | FIELD         |
+| j      | TimestampMillisecond | PRI | NO   |         | TIMESTAMP     |
++--------+----------------------+-----+------+---------+---------------+
+
+INSERT INTO test_alt_table_default (h, j) VALUES (3, 0), (4, 1);
+
+Affected Rows: 2
+
+SELECT * FROM test_alt_table_default ORDER BY h;
+
++---+---+-------------------------+
+| h | i | j                       |
++---+---+-------------------------+
+| 1 | 0 | 1970-01-01T00:00:00     |
+| 2 | 0 | 1970-01-01T00:00:00.001 |
+| 3 | 0 | 1970-01-01T00:00:00     |
+| 4 | 0 | 1970-01-01T00:00:00.001 |
++---+---+-------------------------+
+
+ALTER TABLE test_alt_table_default MODIFY COLUMN i STRING;
+
+Affected Rows: 0
+
+INSERT INTO test_alt_table_default (h, j) VALUES (5, 0);
+
+Affected Rows: 1
+
+INSERT INTO test_alt_table_default (h, i, j) VALUES (6, "word" ,1);
+
+Affected Rows: 1
+
+SELECT * FROM test_alt_table_default ORDER BY h;
+
++---+------+-------------------------+
+| h | i    | j                       |
++---+------+-------------------------+
+| 1 | 0    | 1970-01-01T00:00:00     |
+| 2 | 0    | 1970-01-01T00:00:00.001 |
+| 3 | 0    | 1970-01-01T00:00:00     |
+| 4 | 0    | 1970-01-01T00:00:00.001 |
+| 5 | 0    | 1970-01-01T00:00:00     |
+| 6 | word | 1970-01-01T00:00:00.001 |
++---+------+-------------------------+
+
+DROP TABLE test_alt_table_default;
+
+Affected Rows: 0
+
 -- to test if same name column can be added
 CREATE TABLE phy (ts timestamp time index, val double) engine = metric with ("physical_metric_table" = "");
 

tests/cases/standalone/common/alter/alter_table.sql

@@ -25,10 +25,51 @@ ALTER TABLE test_alt_table ADD COLUMN n interval;
 -- Should fail issue #5422
 ALTER TABLE test_alt_table MODIFY COLUMN m interval;
 
+INSERT INTO test_alt_table (h, i, j, m, dt) VALUES (42, 42, 0, 11, 0);
+
+ALTER TABLE test_alt_table MODIFY COLUMN m Float64;
+
+SELECT * FROM test_alt_table;
+
+ALTER TABLE test_alt_table MODIFY COLUMN m INTEGER;
+
+SELECT * FROM test_alt_table;
+
+ALTER TABLE test_alt_table MODIFY COLUMN m BOOLEAN;
+
+SELECT * FROM test_alt_table;
+
 DESC TABLE test_alt_table;
 
 DROP TABLE test_alt_table;
 
+-- test if column with default value can change type properly
+CREATE TABLE test_alt_table_default(h INTEGER, i INTEGER DEFAULT 0, j TIMESTAMP TIME INDEX, PRIMARY KEY (h));
+
+ALTER TABLE test_alt_table_default MODIFY COLUMN i BOOLEAN;
+
+INSERT INTO test_alt_table_default (h, j) VALUES (1, 0), (2, 1);
+
+SELECT * FROM test_alt_table_default ORDER BY h;
+
+ALTER TABLE test_alt_table_default MODIFY COLUMN i INTEGER;
+
+DESC TABLE test_alt_table_default;
+
+INSERT INTO test_alt_table_default (h, j) VALUES (3, 0), (4, 1);
+
+SELECT * FROM test_alt_table_default ORDER BY h;
+
+ALTER TABLE test_alt_table_default MODIFY COLUMN i STRING;
+
+INSERT INTO test_alt_table_default (h, j) VALUES (5, 0);
+
+INSERT INTO test_alt_table_default (h, i, j) VALUES (6, "word" ,1);
+
+SELECT * FROM test_alt_table_default ORDER BY h;
+
+DROP TABLE test_alt_table_default;
+
 -- to test if same name column can be added
 CREATE TABLE phy (ts timestamp time index, val double) engine = metric with ("physical_metric_table" = "");