From 99dad9615e6ff46fd19164bf17e680fbffbd4285 Mon Sep 17 00:00:00 2001 From: ZhuZiyi Date: Thu, 14 Sep 2023 19:16:42 +0800 Subject: [PATCH 1/4] feat: reserve internal column Signed-off-by: ZhuZiyi --- src/store-api/src/metadata.rs | 11 +++++++++++ src/store-api/src/storage/consts.rs | 11 +++++++++++ 2 files changed, 22 insertions(+) diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index 8e9e8a060999..e08b0f138e93 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -34,6 +34,7 @@ use snafu::{ensure, Location, OptionExt, ResultExt, Snafu}; use crate::region_request::{AddColumn, AddColumnLocation, AlterKind}; use crate::storage::{ColumnId, RegionId}; +use crate::storage::consts::is_internal_column; pub type Result = std::result::Result; @@ -368,6 +369,16 @@ impl RegionMetadata { ); } + ensure!( + !is_internal_column(&column_metadata.column_schema.name), + InvalidMetaSnafu { + reason: format!( + "{} is internal column name that can not be used", + column_metadata.column_schema.name + ), + } + ); + Ok(()) } } diff --git a/src/store-api/src/storage/consts.rs b/src/store-api/src/storage/consts.rs index c092f4bf204f..35d40ce52e7d 100644 --- a/src/store-api/src/storage/consts.rs +++ b/src/store-api/src/storage/consts.rs @@ -84,6 +84,17 @@ pub const OP_TYPE_COLUMN_NAME: &str = "__op_type"; /// Name for reserved column: primary_key pub const PRIMARY_KEY_COLUMN_NAME: &str = "__primary_key"; +/// Internal Column Name +static INTERNAL_COLUMN_VEC: [&str; 3] = [ + SEQUENCE_COLUMN_NAME, + OP_TYPE_COLUMN_NAME, + PRIMARY_KEY_COLUMN_NAME, +]; + +pub fn is_internal_column(name: &str) -> bool { + INTERNAL_COLUMN_VEC.iter().any(|&column| name.contains(column)) +} + // ----------------------------------------------------------------------------- // ---------- Default options -------------------------------------------------- From ee9e7fa4b388142e0533ff758032a067a02e1457 Mon Sep 17 00:00:00 2001 From: ZhuZiyi Date: Thu, 14 Sep 2023 22:13:12 +0800 Subject: [PATCH 2/4] test: add function test Signed-off-by: ZhuZiyi --- src/store-api/src/metadata.rs | 22 +++++++++++++++++++++- src/store-api/src/storage/consts.rs | 14 ++++++++++++-- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index e08b0f138e93..1638477be21c 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -33,8 +33,8 @@ use serde::{Deserialize, Deserializer, Serialize}; use snafu::{ensure, Location, OptionExt, ResultExt, Snafu}; use crate::region_request::{AddColumn, AddColumnLocation, AlterKind}; -use crate::storage::{ColumnId, RegionId}; use crate::storage::consts::is_internal_column; +use crate::storage::{ColumnId, RegionId}; pub type Result = std::result::Result; @@ -1009,4 +1009,24 @@ mod test { let err = builder.build().unwrap_err(); assert_eq!(StatusCode::InvalidArguments, err.status_code()); } + + #[test] + fn test_invalid_column_name() { + let mut builder = create_builder(); + builder.push_column_metadata(ColumnMetadata { + column_schema: ColumnSchema::new( + "__sequence", + ConcreteDataType::timestamp_millisecond_datatype(), + false, + ), + semantic_type: SemanticType::Timestamp, + column_id: 1, + }); + let err = builder.build().unwrap_err(); + assert!( + err.to_string() + .contains("internal column name that can not be used"), + "unexpected err: {err}", + ); + } } diff --git a/src/store-api/src/storage/consts.rs b/src/store-api/src/storage/consts.rs index 35d40ce52e7d..76400f051b3f 100644 --- a/src/store-api/src/storage/consts.rs +++ b/src/store-api/src/storage/consts.rs @@ -84,7 +84,7 @@ pub const OP_TYPE_COLUMN_NAME: &str = "__op_type"; /// Name for reserved column: primary_key pub const PRIMARY_KEY_COLUMN_NAME: &str = "__primary_key"; -/// Internal Column Name +/// Internal Column Name static INTERNAL_COLUMN_VEC: [&str; 3] = [ SEQUENCE_COLUMN_NAME, OP_TYPE_COLUMN_NAME, @@ -92,7 +92,7 @@ static INTERNAL_COLUMN_VEC: [&str; 3] = [ ]; pub fn is_internal_column(name: &str) -> bool { - INTERNAL_COLUMN_VEC.iter().any(|&column| name.contains(column)) + INTERNAL_COLUMN_VEC.contains(&name) } // ----------------------------------------------------------------------------- @@ -115,4 +115,14 @@ mod tests { assert_eq!(0x80000001, ReservedColumnId::sequence()); assert_eq!(0x80000002, ReservedColumnId::op_type()); } + + #[test] + fn test_is_internal_column() { + assert!(is_internal_column(SEQUENCE_COLUMN_NAME)); + assert!(is_internal_column(OP_TYPE_COLUMN_NAME)); + assert!(is_internal_column(PRIMARY_KEY_COLUMN_NAME)); + + let vaild_name: String = "GreptimeDB".to_string() + SEQUENCE_COLUMN_NAME; + assert!(!is_internal_column(&vaild_name)); + } } From 92309930a0951badf32b063279cde6d5664c28b4 Mon Sep 17 00:00:00 2001 From: ZhuZiyi Date: Fri, 15 Sep 2023 09:59:05 +0800 Subject: [PATCH 3/4] fix: spell typos Signed-off-by: ZhuZiyi --- src/store-api/src/storage/consts.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/store-api/src/storage/consts.rs b/src/store-api/src/storage/consts.rs index 76400f051b3f..b7f8e1281a89 100644 --- a/src/store-api/src/storage/consts.rs +++ b/src/store-api/src/storage/consts.rs @@ -122,7 +122,7 @@ mod tests { assert!(is_internal_column(OP_TYPE_COLUMN_NAME)); assert!(is_internal_column(PRIMARY_KEY_COLUMN_NAME)); - let vaild_name: String = "GreptimeDB".to_string() + SEQUENCE_COLUMN_NAME; - assert!(!is_internal_column(&vaild_name)); + let valid_name: String = "GreptimeDB".to_string() + SEQUENCE_COLUMN_NAME; + assert!(!is_internal_column(&valid_name)); } } From 32b35d34e25b6a2b85486d234351baa426bd8ccd Mon Sep 17 00:00:00 2001 From: ZhuZiyi Date: Fri, 15 Sep 2023 11:08:07 +0800 Subject: [PATCH 4/4] chore: resolve conversation Signed-off-by: ZhuZiyi --- src/store-api/src/storage/consts.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/store-api/src/storage/consts.rs b/src/store-api/src/storage/consts.rs index b7f8e1281a89..284d98177a46 100644 --- a/src/store-api/src/storage/consts.rs +++ b/src/store-api/src/storage/consts.rs @@ -118,11 +118,15 @@ mod tests { #[test] fn test_is_internal_column() { + // contain internal column names assert!(is_internal_column(SEQUENCE_COLUMN_NAME)); assert!(is_internal_column(OP_TYPE_COLUMN_NAME)); assert!(is_internal_column(PRIMARY_KEY_COLUMN_NAME)); - let valid_name: String = "GreptimeDB".to_string() + SEQUENCE_COLUMN_NAME; - assert!(!is_internal_column(&valid_name)); + // don't contain internal column names + assert!(!is_internal_column("my__column")); + assert!(!is_internal_column("my__sequence")); + assert!(!is_internal_column("my__op_type")); + assert!(!is_internal_column("my__primary_key")); } }