diff --git a/src/datatypes/src/schema/constraint.rs b/src/datatypes/src/schema/constraint.rs index 560500810f..c6551687a4 100644 --- a/src/datatypes/src/schema/constraint.rs +++ b/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 { + 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 diff --git a/src/mito2/src/error.rs b/src/mito2/src/error.rs index 9c9c78f07e..53ad243237 100644 --- a/src/mito2/src/error.rs +++ b/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))] diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index f8df4745d4..e9dfae10bc 100644 --- a/src/store-api/src/metadata.rs +++ b/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) { + fn modify_column_types(&mut self, columns: Vec) -> 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, diff --git a/tests-fuzz/src/validator/column.rs b/tests-fuzz/src/validator/column.rs index 8642a275a4..1bf8274e92 100644 --- a/tests-fuzz/src/validator/column.rs +++ b/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 { + 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}; diff --git a/tests-fuzz/targets/ddl/fuzz_alter_table.rs b/tests-fuzz/targets/ddl/fuzz_alter_table.rs index adff64827c..38357e7ef5 100644 --- a/tests-fuzz/targets/ddl/fuzz_alter_table.rs +++ b/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(rng: &mut R) -> Result( + rng: &mut R, + create_table_expr: &CreateTableExpr, +) -> Result { + 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( 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, diff --git a/tests/cases/standalone/common/alter/alter_table.result b/tests/cases/standalone/common/alter/alter_table.result index dff58e802d..e441b59c22 100644 --- a/tests/cases/standalone/common/alter/alter_table.result +++ b/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" = ""); diff --git a/tests/cases/standalone/common/alter/alter_table.sql b/tests/cases/standalone/common/alter/alter_table.sql index ef8c5ff539..56db60f062 100644 --- a/tests/cases/standalone/common/alter/alter_table.sql +++ b/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" = "");