allow physical region alter region options

GreptimeTeam · Nov 25, 2024 · a83071e · a83071e
1 parent a6571d3
commit a83071e
Show file tree

Hide file tree

Showing 5 changed files with 113 additions and 13 deletions.
diff --git a/src/metric-engine/src/data_region.rs b/src/metric-engine/src/data_region.rs
@@ -27,9 +27,10 @@ use store_api::storage::consts::ReservedColumnId;
 use store_api::storage::{ConcreteDataType, RegionId};
 
 use crate::error::{
-    ColumnTypeMismatchSnafu, MitoReadOperationSnafu, MitoWriteOperationSnafu, Result,
+    ColumnTypeMismatchSnafu, ForbiddenPhysicalAlterSnafu, MitoReadOperationSnafu,
+    MitoWriteOperationSnafu, Result,
 };
-use crate::metrics::MITO_DDL_DURATION;
+use crate::metrics::{FORBIDDEN_OPERATION_COUNT, MITO_DDL_DURATION};
 use crate::utils;
 
 const MAX_RETRIES: usize = 5;
@@ -186,6 +187,30 @@ impl DataRegion {
             .context(MitoReadOperationSnafu)?;
         Ok(metadata.column_metadatas.clone())
     }
+
+    pub async fn alter_region_options(
+        &self,
+        region_id: RegionId,
+        request: RegionAlterRequest,
+    ) -> Result<AffectedRows> {
+        match request.kind {
+            AlterKind::SetRegionOptions { options: _ }
+            | AlterKind::UnsetRegionOptions { keys: _ } => {
+                let region_id = utils::to_data_region_id(region_id);
+                self.mito
+                    .handle_request(region_id, RegionRequest::Alter(request))
+                    .await
+                    .context(MitoWriteOperationSnafu)
+                    .map(|result| result.affected_rows)
+            }
+            _ => {
+                info!("Metric region received alter request {request:?} on physical region {region_id:?}");
+                FORBIDDEN_OPERATION_COUNT.inc();
+
+                ForbiddenPhysicalAlterSnafu.fail()
+            }
+        }
+    }
 }
 
 #[cfg(test)]

diff --git a/src/metric-engine/src/engine.rs b/src/metric-engine/src/engine.rs
@@ -96,9 +96,10 @@ use crate::utils;
 /// |    Read    |       ✅        |        ✅        |
 /// |   Close    |       ✅        |        ✅        |
 /// |    Open    |       ✅        |        ✅        |
-/// |   Alter    |       ✅        |        ❌        |
+/// |   Alter    |       ✅        |        ❓*       |
 ///
 /// *: Physical region can be dropped only when all related logical regions are dropped.
+/// *: Alter: Physical regions only support altering region options.
 ///
 /// ## Internal Columns
 ///

diff --git a/src/metric-engine/src/engine/alter.rs b/src/metric-engine/src/engine/alter.rs
@@ -14,18 +14,15 @@
 
 use std::collections::HashMap;
 
-use common_telemetry::{error, info};
+use common_telemetry::error;
 use snafu::{OptionExt, ResultExt};
 use store_api::metadata::ColumnMetadata;
 use store_api::metric_engine_consts::ALTER_PHYSICAL_EXTENSION_KEY;
 use store_api::region_request::{AffectedRows, AlterKind, RegionAlterRequest};
 use store_api::storage::RegionId;
 
 use crate::engine::MetricEngineInner;
-use crate::error::{
-    ForbiddenPhysicalAlterSnafu, LogicalRegionNotFoundSnafu, Result, SerializeColumnMetadataSnafu,
-};
-use crate::metrics::FORBIDDEN_OPERATION_COUNT;
+use crate::error::{LogicalRegionNotFoundSnafu, Result, SerializeColumnMetadataSnafu};
 use crate::utils::{to_data_region_id, to_metadata_region_id};
 
 impl MetricEngineInner {
@@ -150,20 +147,22 @@ impl MetricEngineInner {
         region_id: RegionId,
         request: RegionAlterRequest,
     ) -> Result<()> {
-        info!("Metric region received alter request {request:?} on physical region {region_id:?}");
-        FORBIDDEN_OPERATION_COUNT.inc();
-
-        ForbiddenPhysicalAlterSnafu.fail()
+        self.data_region
+            .alter_region_options(region_id, request)
+            .await?;
+        Ok(())
     }
 }
 
 #[cfg(test)]
 mod test {
+    use std::time::Duration;
+
     use api::v1::SemanticType;
     use datatypes::data_type::ConcreteDataType;
     use datatypes::schema::ColumnSchema;
     use store_api::metadata::ColumnMetadata;
-    use store_api::region_request::AddColumn;
+    use store_api::region_request::{AddColumn, SetRegionOption};
 
     use super::*;
     use crate::test_util::TestEnv;
@@ -204,6 +203,18 @@ mod test {
             "Alter request to physical region is forbidden".to_string()
         );
 
+        // alter physical region's option should work
+        let alter_region_option_request = RegionAlterRequest {
+            schema_version: 0,
+            kind: AlterKind::SetRegionOptions {
+                options: vec![SetRegionOption::TTL(Duration::from_secs(500))],
+            },
+        };
+        let result = engine_inner
+            .alter_physical_region(physical_region_id, alter_region_option_request.clone())
+            .await;
+        assert!(result.is_ok());
+
         // alter logical region
         let metadata_region = env.metadata_region();
         let logical_region_id = env.default_logical_region_id();

diff --git a/tests/cases/standalone/common/alter/alter_table_options.result b/tests/cases/standalone/common/alter/alter_table_options.result
@@ -281,3 +281,54 @@ DROP TABLE ato;
 
 Affected Rows: 0
 
+CREATE TABLE phy (ts timestamp time index, val double) engine=metric with ("physical_metric_table" = "");
+
+Affected Rows: 0
+
+ALTER TABLE phy set ttl='2years';
+
+Affected Rows: 0
+
+SHOW CREATE TABLE phy;
+
++-------+------------------------------------+
+| Table | Create Table                       |
++-------+------------------------------------+
+| phy   | CREATE TABLE IF NOT EXISTS "phy" ( |
+|       |   "ts" TIMESTAMP(3) NOT NULL,      |
+|       |   "val" DOUBLE NULL,               |
+|       |   TIME INDEX ("ts")                |
+|       | )                                  |
+|       |                                    |
+|       | ENGINE=metric                      |
+|       | WITH(                              |
+|       |   physical_metric_table = '',      |
+|       |   ttl = '2years'                   |
+|       | )                                  |
++-------+------------------------------------+
+
+ALTER TABLE phy UNSET 'ttl';
+
+Affected Rows: 0
+
+SHOW CREATE TABLE phy;
+
++-------+------------------------------------+
+| Table | Create Table                       |
++-------+------------------------------------+
+| phy   | CREATE TABLE IF NOT EXISTS "phy" ( |
+|       |   "ts" TIMESTAMP(3) NOT NULL,      |
+|       |   "val" DOUBLE NULL,               |
+|       |   TIME INDEX ("ts")                |
+|       | )                                  |
+|       |                                    |
+|       | ENGINE=metric                      |
+|       | WITH(                              |
+|       |   physical_metric_table = ''       |
+|       | )                                  |
++-------+------------------------------------+
+
+DROP TABLE phy;
+
+Affected Rows: 0
+
diff --git a/tests/cases/standalone/common/alter/alter_table_options.sql b/tests/cases/standalone/common/alter/alter_table_options.sql
@@ -60,3 +60,15 @@ SHOW CREATE TABLE ato;
 SHOW CREATE TABLE ato;
 
 DROP TABLE ato;
+
+CREATE TABLE phy (ts timestamp time index, val double) engine=metric with ("physical_metric_table" = "");
+
+ALTER TABLE phy set ttl='2years';
+
+SHOW CREATE TABLE phy;
+
+ALTER TABLE phy UNSET 'ttl';
+
+SHOW CREATE TABLE phy;
+
+DROP TABLE phy;