refactor: better check&explain

GreptimeTeam · Nov 13, 2024 · 2360dcb · 2360dcb
1 parent 6000e4f
commit 2360dcb
Show file tree

Hide file tree

Showing 6 changed files with 34 additions and 100 deletions.
diff --git 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::<Result<_>>()?;
 
-        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
@@ -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::<HashMap<_, _>>();
-
         // 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
@@ -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,27 +297,28 @@ 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::<HashMap<_, _>>();
 
-        // 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,
                     col,
                     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
@@ -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<Option<String>> {
         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
@@ -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
@@ -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;
+DROP TABLE phy;