diff --git a/src/metric-engine/src/data_region.rs b/src/metric-engine/src/data_region.rs index de6083200bbc..87db1536319c 100644 --- a/src/metric-engine/src/data_region.rs +++ b/src/metric-engine/src/data_region.rs @@ -15,8 +15,7 @@ use api::v1::SemanticType; use common_error::ext::ErrorExt; use common_error::status_code::StatusCode; -use common_telemetry::info; -use common_telemetry::tracing::warn; +use common_telemetry::{debug, info, warn}; use mito2::engine::MitoEngine; use snafu::ResultExt; use store_api::metadata::ColumnMetadata; @@ -150,7 +149,7 @@ impl DataRegion { }) .collect::>()?; - info!("Adding (Column id assigned) columns {new_columns:?} to region {region_id:?}"); + debug!("Adding (Column id assigned) columns {new_columns:?} to region {region_id:?}"); // assemble alter request let alter_request = RegionRequest::Alter(RegionAlterRequest { schema_version: version, diff --git a/src/metric-engine/src/engine/alter.rs b/src/metric-engine/src/engine/alter.rs index dc2205266aa8..ea32c98b1852 100644 --- a/src/metric-engine/src/engine/alter.rs +++ b/src/metric-engine/src/engine/alter.rs @@ -23,8 +23,7 @@ use store_api::storage::RegionId; use crate::engine::MetricEngineInner; use crate::error::{ - ColumnNotFoundAfterAlterSnafu, ForbiddenPhysicalAlterSnafu, LogicalRegionNotFoundSnafu, Result, - SerializeColumnMetadataSnafu, + ForbiddenPhysicalAlterSnafu, LogicalRegionNotFoundSnafu, Result, SerializeColumnMetadataSnafu, }; use crate::metrics::FORBIDDEN_OPERATION_COUNT; use crate::utils::{to_data_region_id, to_metadata_region_id}; @@ -126,35 +125,16 @@ impl MetricEngineInner { ) .await?; - // check physical region id again, since it may have been altered and column ids might be different than - // the one we have - let after_alter_phy_table = self - .data_region - .physical_columns(physical_region_id) - .await?; - let after_alter_cols = after_alter_phy_table - .iter() - .map(|col| (col.column_schema.name.clone(), col.clone())) - .collect::>(); - // register columns to logical region - // we need to use modified column metadata from physical region, since it may have been altered(especially column id) - for col in columns { - if let Some(metadata) = after_alter_cols.get(&col.column_metadata.column_schema.name) { - self.metadata_region - .add_column(metadata_region_id, logical_region_id, metadata) - .await?; - } else { - error!( - "Column {} not found after altering physical region {:?}", - col.column_metadata.column_schema.name, data_region_id - ); - - ColumnNotFoundAfterAlterSnafu { - column_name: col.column_metadata.column_schema.name.clone(), - } - .fail()?; - } + // note here we don't use `columns` directly but concat `existing_columns` with `columns_to_add` to get correct metadata + // about already existing columns + for metadata in existing_columns + .into_iter() + .chain(columns_to_add.into_iter()) + { + self.metadata_region + .add_column(metadata_region_id, logical_region_id, &metadata) + .await?; } // invalid logical column cache diff --git a/src/metric-engine/src/engine/create.rs b/src/metric-engine/src/engine/create.rs index 30f401520264..b90d49005810 100644 --- a/src/metric-engine/src/engine/create.rs +++ b/src/metric-engine/src/engine/create.rs @@ -273,7 +273,6 @@ impl MetricEngineInner { /// /// `new_columns` MUST NOT pre-exist in the physical region. Or the results will be wrong column id for the new columns. /// - /// TODO(discord9): change this to actually return the actually added physical columns pub(crate) async fn add_columns_to_physical_data_region( &self, data_region_id: RegionId, @@ -298,19 +297,20 @@ impl MetricEngineInner { // correct the column id let after_alter_physical_schema = self.data_region.physical_columns(data_region_id).await?; let after_alter_physical_schema_map = after_alter_physical_schema - .into_iter() - .map(|metadata| (metadata.column_schema.name.clone(), metadata)) + .iter() + .map(|metadata| (metadata.column_schema.name.as_str(), metadata)) .collect::>(); - // check to make sure column ids are not mismatched + // double check to make sure column ids are not mismatched + // shouldn't be a expensive operation, given it only query for physical columns for col in new_columns.iter_mut() { let column_metadata = after_alter_physical_schema_map - .get(&col.column_schema.name) + .get(&col.column_schema.name.as_str()) .with_context(|| ColumnNotFoundSnafu { name: &col.column_schema.name, region_id: data_region_id, })?; - if col != column_metadata { + if col != *column_metadata { warn!( "Add already existing columns with different column metadata to physical region({:?}): new column={:?}, old column={:?}", data_region_id, @@ -318,7 +318,7 @@ impl MetricEngineInner { column_metadata ); // update to correct metadata - *col = column_metadata.clone(); + *col = (*column_metadata).clone(); } } diff --git a/src/metric-engine/src/metadata_region.rs b/src/metric-engine/src/metadata_region.rs index 2ecce12e17d6..e440eb1765f7 100644 --- a/src/metric-engine/src/metadata_region.rs +++ b/src/metric-engine/src/metadata_region.rs @@ -211,7 +211,7 @@ impl MetadataRegion { } /// Check if the given column exists. Return the semantic type if exists. - #[allow(unused)] + #[cfg(test)] pub async fn column_semantic_type( &self, physical_region_id: RegionId, @@ -374,6 +374,7 @@ impl MetadataRegion { /// Retrieves the value associated with the given key in the specified region. /// Returns `Ok(None)` if the key is not found. + #[cfg(test)] pub async fn get(&self, region_id: RegionId, key: &str) -> Result> { let scan_req = Self::build_read_request(key); let record_batch_stream = self diff --git a/tests/cases/standalone/common/alter/alter_table.result b/tests/cases/standalone/common/alter/alter_table.result index 0b8af61c9800..120e7695d03f 100644 --- a/tests/cases/standalone/common/alter/alter_table.result +++ b/tests/cases/standalone/common/alter/alter_table.result @@ -1,9 +1,4 @@ -CREATE TABLE test_alt_table( - h INTEGER, - i INTEGER, - j TIMESTAMP TIME INDEX, - PRIMARY KEY (h, i) -); +CREATE TABLE test_alt_table(h INTEGER, i INTEGER, j TIMESTAMP TIME INDEX, PRIMARY KEY (h, i)); Affected Rows: 0 @@ -17,20 +12,13 @@ DESC TABLE test_alt_table; | j | TimestampMillisecond | PRI | NO | | TIMESTAMP | +--------+----------------------+-----+------+---------+---------------+ -INSERT INTO - test_alt_table -VALUES - (1, 1, 0), - (2, 2, 1); +INSERT INTO test_alt_table VALUES (1, 1, 0), (2, 2, 1); Affected Rows: 2 -- TODO: It may result in an error if `k` is with type INTEGER. -- Error: 3001(EngineExecuteQuery), Invalid argument error: column types must match schema types, expected Int32 but found Utf8 at column index 3 -ALTER TABLE - test_alt_table -ADD - COLUMN k STRING PRIMARY KEY; +ALTER TABLE test_alt_table ADD COLUMN k STRING PRIMARY KEY; Affected Rows: 0 @@ -45,10 +33,7 @@ DESC TABLE test_alt_table; | k | String | PRI | YES | | TAG | +--------+----------------------+-----+------+---------+---------------+ -SELECT - * -FROM - test_alt_table; +SELECT * FROM test_alt_table; +---+---+-------------------------+---+ | h | i | j | k | @@ -57,12 +42,7 @@ FROM | 2 | 2 | 1970-01-01T00:00:00.001 | | +---+---+-------------------------+---+ -SELECT - * -FROM - test_alt_table -WHERE - i = 1; +SELECT * FROM test_alt_table WHERE i = 1; +---+---+---------------------+---+ | h | i | j | k | @@ -71,10 +51,7 @@ WHERE +---+---+---------------------+---+ -- SQLNESS ARG restart=true -ALTER TABLE - test_alt_table -ADD - COLUMN m INTEGER; +ALTER TABLE test_alt_table ADD COLUMN m INTEGER; Affected Rows: 0 diff --git a/tests/cases/standalone/common/alter/alter_table.sql b/tests/cases/standalone/common/alter/alter_table.sql index d560e2f208cf..7f3e0b664038 100644 --- a/tests/cases/standalone/common/alter/alter_table.sql +++ b/tests/cases/standalone/common/alter/alter_table.sql @@ -1,44 +1,21 @@ -CREATE TABLE test_alt_table( - h INTEGER, - i INTEGER, - j TIMESTAMP TIME INDEX, - PRIMARY KEY (h, i) -); +CREATE TABLE test_alt_table(h INTEGER, i INTEGER, j TIMESTAMP TIME INDEX, PRIMARY KEY (h, i)); DESC TABLE test_alt_table; -INSERT INTO - test_alt_table -VALUES - (1, 1, 0), - (2, 2, 1); +INSERT INTO test_alt_table VALUES (1, 1, 0), (2, 2, 1); -- TODO: It may result in an error if `k` is with type INTEGER. -- Error: 3001(EngineExecuteQuery), Invalid argument error: column types must match schema types, expected Int32 but found Utf8 at column index 3 -ALTER TABLE - test_alt_table -ADD - COLUMN k STRING PRIMARY KEY; +ALTER TABLE test_alt_table ADD COLUMN k STRING PRIMARY KEY; DESC TABLE test_alt_table; -SELECT - * -FROM - test_alt_table; +SELECT * FROM test_alt_table; -SELECT - * -FROM - test_alt_table -WHERE - i = 1; +SELECT * FROM test_alt_table WHERE i = 1; -- SQLNESS ARG restart=true -ALTER TABLE - test_alt_table -ADD - COLUMN m INTEGER; +ALTER TABLE test_alt_table ADD COLUMN m INTEGER; DESC TABLE test_alt_table; @@ -104,4 +81,4 @@ DROP TABLE t1; DROP TABLE t2; -DROP TABLE phy; \ No newline at end of file +DROP TABLE phy;