From 24886b9530cffda3d3c5e735d20872ad154f82f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?x=C2=B3u=C2=B3?= <751736277@qq.com> Date: Sat, 23 Mar 2024 01:43:20 +0800 Subject: [PATCH 1/8] test: add a parameter type mismatch test case to sql integration test (#3568) --- tests-integration/tests/sql.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests-integration/tests/sql.rs b/tests-integration/tests/sql.rs index 5b9e1e2f35..7cff590c39 100644 --- a/tests-integration/tests/sql.rs +++ b/tests-integration/tests/sql.rs @@ -197,6 +197,23 @@ pub async fn test_mysql_crud(store_type: StorageType) { assert_eq!(ret, 6); } + // parameter type mismatch + let query_re = sqlx::query("select i from demo where i = ?") + .bind("test") + .fetch_all(&pool) + .await; + assert!(query_re.is_err()); + assert_eq!( + query_re + .err() + .unwrap() + .into_database_error() + .unwrap() + .downcast::() + .code(), + Some("22007") + ); + let _ = sqlx::query("delete from demo") .execute(&pool) .await From 2b2fd80bf475f04f284150b935a7108a8d1273d5 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Sat, 23 Mar 2024 17:31:16 +0800 Subject: [PATCH 2/8] feat: return new added columns in region server's extension response (#3533) * feat: adapt the new proto response Signed-off-by: Ruihang Xia * update interfaces Signed-off-by: Ruihang Xia * write columns to extension Signed-off-by: Ruihang Xia * use physical column's schema Signed-off-by: Ruihang Xia * sort logical columns by name Signed-off-by: Ruihang Xia * format code Signed-off-by: Ruihang Xia * return physical table's column Signed-off-by: Ruihang Xia * Update src/common/meta/src/datanode_manager.rs Co-authored-by: JeremyHi * implement sort column logic Signed-off-by: Ruihang Xia * proxy create table procedure to create logical table Signed-off-by: Ruihang Xia * add unit test for sort_columns Signed-off-by: Ruihang Xia * update sqlness cases Signed-off-by: Ruihang Xia --------- Signed-off-by: Ruihang Xia Co-authored-by: JeremyHi --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/client/src/region.rs | 36 ++--- src/common/meta/src/datanode_manager.rs | 29 +++- .../meta/src/ddl/create_logical_tables.rs | 8 +- .../src/ddl/tests/create_logical_tables.rs | 7 +- src/common/meta/src/ddl/tests/create_table.rs | 13 +- src/common/meta/src/rpc/ddl.rs | 152 +++++++++++++++++- src/common/meta/src/test_util.rs | 8 +- src/datanode/src/region_server.rs | 35 ++-- src/datanode/src/tests.rs | 10 +- src/datatypes/src/schema/column_schema.rs | 11 ++ src/file-engine/src/engine.rs | 11 +- src/frontend/src/instance/standalone.rs | 8 +- src/meta-srv/src/procedure/utils.rs | 1 + src/metric-engine/src/data_region.rs | 36 +++-- src/metric-engine/src/engine.rs | 39 +++-- src/metric-engine/src/engine/alter.rs | 34 +++- src/metric-engine/src/engine/create.rs | 81 ++++++++-- src/metric-engine/src/engine/put.rs | 8 +- src/metric-engine/src/engine/read.rs | 6 +- .../src/engine/region_metadata.rs | 3 +- src/metric-engine/src/error.rs | 19 ++- src/metric-engine/src/metadata_region.rs | 2 +- src/mito2/src/engine.rs | 5 +- src/mito2/src/engine/basic_test.rs | 4 +- src/mito2/src/engine/compaction_test.rs | 16 +- src/mito2/src/test_util.rs | 12 +- src/operator/src/delete.rs | 5 +- src/operator/src/insert.rs | 5 +- src/operator/src/request.rs | 5 +- src/operator/src/statement/ddl.rs | 17 ++ src/store-api/src/metadata.rs | 10 ++ src/store-api/src/metric_engine_consts.rs | 4 + src/store-api/src/region_engine.rs | 20 ++- src/table/src/metadata.rs | 6 + tests-integration/src/prom_store.rs | 8 +- .../common/create/create_metric_table.result | 4 +- .../common/insert/logical_metric_table.result | 28 ++-- .../common/insert/logical_metric_table.sql | 4 +- 40 files changed, 543 insertions(+), 171 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 385ae145c4..9a12234862 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3870,7 +3870,7 @@ checksum = "d2fabcfbdc87f4758337ca535fb41a6d701b65693ce38287d856d1674551ec9b" [[package]] name = "greptime-proto" version = "0.1.0" -source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=96f1f0404f421ee560a4310c73c5071e49168168#96f1f0404f421ee560a4310c73c5071e49168168" +source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=349cb385583697f41010dabeb3c106d58f9599b4#349cb385583697f41010dabeb3c106d58f9599b4" dependencies = [ "prost 0.12.3", "serde", diff --git a/Cargo.toml b/Cargo.toml index 4681c65354..509d7f84c8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -103,7 +103,7 @@ etcd-client = "0.12" fst = "0.4.7" futures = "0.3" futures-util = "0.3" -greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "96f1f0404f421ee560a4310c73c5071e49168168" } +greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "349cb385583697f41010dabeb3c106d58f9599b4" } humantime-serde = "1.1" itertools = "0.10" lazy_static = "1.4" diff --git a/src/client/src/region.rs b/src/client/src/region.rs index 819c4453f9..39c5ddddcc 100644 --- a/src/client/src/region.rs +++ b/src/client/src/region.rs @@ -14,7 +14,7 @@ use std::sync::Arc; -use api::v1::region::{QueryRequest, RegionRequest, RegionResponse}; +use api::v1::region::{QueryRequest, RegionRequest}; use api::v1::ResponseHeader; use arc_swap::ArcSwapOption; use arrow_flight::Ticket; @@ -23,7 +23,7 @@ use async_trait::async_trait; use common_error::ext::{BoxedError, ErrorExt}; use common_error::status_code::StatusCode; use common_grpc::flight::{FlightDecoder, FlightMessage}; -use common_meta::datanode_manager::{AffectedRows, Datanode}; +use common_meta::datanode_manager::{Datanode, HandleResponse}; use common_meta::error::{self as meta_error, Result as MetaResult}; use common_recordbatch::error::ExternalSnafu; use common_recordbatch::{RecordBatchStreamWrapper, SendableRecordBatchStream}; @@ -46,7 +46,7 @@ pub struct RegionRequester { #[async_trait] impl Datanode for RegionRequester { - async fn handle(&self, request: RegionRequest) -> MetaResult { + async fn handle(&self, request: RegionRequest) -> MetaResult { self.handle_inner(request).await.map_err(|err| { if err.should_retry() { meta_error::Error::RetryLater { @@ -165,7 +165,7 @@ impl RegionRequester { Ok(Box::pin(record_batch_stream)) } - async fn handle_inner(&self, request: RegionRequest) -> Result { + async fn handle_inner(&self, request: RegionRequest) -> Result { let request_type = request .body .as_ref() @@ -178,10 +178,7 @@ impl RegionRequester { let mut client = self.client.raw_region_client()?; - let RegionResponse { - header, - affected_rows, - } = client + let response = client .handle(request) .await .map_err(|e| { @@ -195,19 +192,20 @@ impl RegionRequester { })? .into_inner(); - check_response_header(header)?; + check_response_header(&response.header)?; - Ok(affected_rows as _) + Ok(HandleResponse::from_region_response(response)) } - pub async fn handle(&self, request: RegionRequest) -> Result { + pub async fn handle(&self, request: RegionRequest) -> Result { self.handle_inner(request).await } } -pub fn check_response_header(header: Option) -> Result<()> { +pub fn check_response_header(header: &Option) -> Result<()> { let status = header - .and_then(|header| header.status) + .as_ref() + .and_then(|header| header.status.as_ref()) .context(IllegalDatabaseResponseSnafu { err_msg: "either response header or status is missing", })?; @@ -221,7 +219,7 @@ pub fn check_response_header(header: Option) -> Result<()> { })?; ServerSnafu { code, - msg: status.err_msg, + msg: status.err_msg.clone(), } .fail() } @@ -236,19 +234,19 @@ mod test { #[test] fn test_check_response_header() { - let result = check_response_header(None); + let result = check_response_header(&None); assert!(matches!( result.unwrap_err(), IllegalDatabaseResponse { .. } )); - let result = check_response_header(Some(ResponseHeader { status: None })); + let result = check_response_header(&Some(ResponseHeader { status: None })); assert!(matches!( result.unwrap_err(), IllegalDatabaseResponse { .. } )); - let result = check_response_header(Some(ResponseHeader { + let result = check_response_header(&Some(ResponseHeader { status: Some(PbStatus { status_code: StatusCode::Success as u32, err_msg: String::default(), @@ -256,7 +254,7 @@ mod test { })); assert!(result.is_ok()); - let result = check_response_header(Some(ResponseHeader { + let result = check_response_header(&Some(ResponseHeader { status: Some(PbStatus { status_code: u32::MAX, err_msg: String::default(), @@ -267,7 +265,7 @@ mod test { IllegalDatabaseResponse { .. } )); - let result = check_response_header(Some(ResponseHeader { + let result = check_response_header(&Some(ResponseHeader { status: Some(PbStatus { status_code: StatusCode::Internal as u32, err_msg: "blabla".to_string(), diff --git a/src/common/meta/src/datanode_manager.rs b/src/common/meta/src/datanode_manager.rs index 4795512f25..1fdc2f314e 100644 --- a/src/common/meta/src/datanode_manager.rs +++ b/src/common/meta/src/datanode_manager.rs @@ -12,9 +12,10 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::collections::HashMap; use std::sync::Arc; -use api::v1::region::{QueryRequest, RegionRequest}; +use api::v1::region::{QueryRequest, RegionRequest, RegionResponse}; pub use common_base::AffectedRows; use common_recordbatch::SendableRecordBatchStream; @@ -25,7 +26,7 @@ use crate::peer::Peer; #[async_trait::async_trait] pub trait Datanode: Send + Sync { /// Handles DML, and DDL requests. - async fn handle(&self, request: RegionRequest) -> Result; + async fn handle(&self, request: RegionRequest) -> Result; /// Handles query requests async fn handle_query(&self, request: QueryRequest) -> Result; @@ -41,3 +42,27 @@ pub trait DatanodeManager: Send + Sync { } pub type DatanodeManagerRef = Arc; + +/// This result struct is derived from [RegionResponse] +#[derive(Debug)] +pub struct HandleResponse { + pub affected_rows: AffectedRows, + pub extension: HashMap>, +} + +impl HandleResponse { + pub fn from_region_response(region_response: RegionResponse) -> Self { + Self { + affected_rows: region_response.affected_rows as _, + extension: region_response.extension, + } + } + + /// Creates one response without extension + pub fn new(affected_rows: AffectedRows) -> Self { + Self { + affected_rows, + extension: Default::default(), + } + } +} diff --git a/src/common/meta/src/ddl/create_logical_tables.rs b/src/common/meta/src/ddl/create_logical_tables.rs index 8831e3aeb2..35a32142e4 100644 --- a/src/common/meta/src/ddl/create_logical_tables.rs +++ b/src/common/meta/src/ddl/create_logical_tables.rs @@ -70,6 +70,7 @@ impl CreateLogicalTablesProcedure { /// - Checks whether physical table exists. /// - Checks whether logical tables exist. /// - Allocates the table ids. + /// - Modify tasks to sort logical columns on their names. /// /// Abort(non-retry): /// - The physical table does not exist. @@ -130,7 +131,7 @@ impl CreateLogicalTablesProcedure { )); } - // Allocates table ids + // Allocates table ids and sort columns on their names. for (task, table_id) in tasks.iter_mut().zip(already_exists_tables_ids.iter()) { let table_id = if let Some(table_id) = table_id { *table_id @@ -141,6 +142,11 @@ impl CreateLogicalTablesProcedure { .await? }; task.set_table_id(table_id); + + // sort columns in task + task.sort_columns(); + + common_telemetry::info!("[DEBUG] sorted task {:?}", task); } self.creator diff --git a/src/common/meta/src/ddl/tests/create_logical_tables.rs b/src/common/meta/src/ddl/tests/create_logical_tables.rs index e6710d7440..7f82d372ca 100644 --- a/src/common/meta/src/ddl/tests/create_logical_tables.rs +++ b/src/common/meta/src/ddl/tests/create_logical_tables.rs @@ -28,6 +28,7 @@ use common_telemetry::debug; use store_api::storage::RegionId; use table::metadata::RawTableInfo; +use crate::datanode_manager::HandleResponse; use crate::ddl::create_logical_tables::CreateLogicalTablesProcedure; use crate::ddl::test_util::create_table::build_raw_table_info_from_expr; use crate::ddl::test_util::{TestColumnDefBuilder, TestCreateTableExprBuilder}; @@ -36,7 +37,7 @@ use crate::error::{Error, Result}; use crate::key::table_route::TableRouteValue; use crate::peer::Peer; use crate::rpc::ddl::CreateTableTask; -use crate::test_util::{new_ddl_context, AffectedRows, MockDatanodeHandler, MockDatanodeManager}; +use crate::test_util::{new_ddl_context, MockDatanodeHandler, MockDatanodeManager}; // Note: this code may be duplicated with others. // However, it's by design, ensures the tests are easy to be modified or added. @@ -332,9 +333,9 @@ pub struct NaiveDatanodeHandler; #[async_trait::async_trait] impl MockDatanodeHandler for NaiveDatanodeHandler { - async fn handle(&self, peer: &Peer, request: RegionRequest) -> Result { + async fn handle(&self, peer: &Peer, request: RegionRequest) -> Result { debug!("Returning Ok(0) for request: {request:?}, peer: {peer:?}"); - Ok(0) + Ok(HandleResponse::new(0)) } async fn handle_query( diff --git a/src/common/meta/src/ddl/tests/create_table.rs b/src/common/meta/src/ddl/tests/create_table.rs index d31c78479d..3040ae6d2f 100644 --- a/src/common/meta/src/ddl/tests/create_table.rs +++ b/src/common/meta/src/ddl/tests/create_table.rs @@ -26,6 +26,7 @@ use common_procedure_test::MockContextProvider; use common_recordbatch::SendableRecordBatchStream; use common_telemetry::debug; +use crate::datanode_manager::HandleResponse; use crate::ddl::create_table::CreateTableProcedure; use crate::ddl::test_util::create_table::build_raw_table_info_from_expr; use crate::ddl::test_util::{TestColumnDefBuilder, TestCreateTableExprBuilder}; @@ -34,11 +35,11 @@ use crate::error::{Error, Result}; use crate::key::table_route::TableRouteValue; use crate::peer::Peer; use crate::rpc::ddl::CreateTableTask; -use crate::test_util::{new_ddl_context, AffectedRows, MockDatanodeHandler, MockDatanodeManager}; +use crate::test_util::{new_ddl_context, MockDatanodeHandler, MockDatanodeManager}; #[async_trait::async_trait] impl MockDatanodeHandler for () { - async fn handle(&self, _peer: &Peer, _request: RegionRequest) -> Result { + async fn handle(&self, _peer: &Peer, _request: RegionRequest) -> Result { unreachable!() } @@ -176,7 +177,7 @@ pub struct RetryErrorDatanodeHandler; #[async_trait::async_trait] impl MockDatanodeHandler for RetryErrorDatanodeHandler { - async fn handle(&self, peer: &Peer, request: RegionRequest) -> Result { + async fn handle(&self, peer: &Peer, request: RegionRequest) -> Result { debug!("Returning retry later for request: {request:?}, peer: {peer:?}"); Err(Error::RetryLater { source: BoxedError::new( @@ -220,7 +221,7 @@ pub struct UnexpectedErrorDatanodeHandler; #[async_trait::async_trait] impl MockDatanodeHandler for UnexpectedErrorDatanodeHandler { - async fn handle(&self, peer: &Peer, request: RegionRequest) -> Result { + async fn handle(&self, peer: &Peer, request: RegionRequest) -> Result { debug!("Returning mock error for request: {request:?}, peer: {peer:?}"); error::UnexpectedSnafu { err_msg: "mock error", @@ -260,9 +261,9 @@ pub struct NaiveDatanodeHandler; #[async_trait::async_trait] impl MockDatanodeHandler for NaiveDatanodeHandler { - async fn handle(&self, peer: &Peer, request: RegionRequest) -> Result { + async fn handle(&self, peer: &Peer, request: RegionRequest) -> Result { debug!("Returning Ok(0) for request: {request:?}, peer: {peer:?}"); - Ok(0) + Ok(HandleResponse::new(0)) } async fn handle_query( diff --git a/src/common/meta/src/rpc/ddl.rs b/src/common/meta/src/rpc/ddl.rs index b88867bd3d..b97924aa9f 100644 --- a/src/common/meta/src/rpc/ddl.rs +++ b/src/common/meta/src/rpc/ddl.rs @@ -22,7 +22,7 @@ use api::v1::meta::{ DropTableTask as PbDropTableTask, DropTableTasks as PbDropTableTasks, Partition, ProcedureId, TruncateTableTask as PbTruncateTableTask, }; -use api::v1::{AlterExpr, CreateTableExpr, DropTableExpr, TruncateTableExpr}; +use api::v1::{AlterExpr, CreateTableExpr, DropTableExpr, SemanticType, TruncateTableExpr}; use base64::engine::general_purpose; use base64::Engine as _; use prost::Message; @@ -368,6 +368,44 @@ impl CreateTableTask { pub fn set_table_id(&mut self, table_id: TableId) { self.table_info.ident.table_id = table_id; } + + /// Sort the columns in [CreateTableExpr] and [RawTableInfo]. + /// + /// This function won't do any check or verification. Caller should + /// ensure this task is valid. + pub fn sort_columns(&mut self) { + // sort create table expr + // sort column_defs by name + self.create_table + .column_defs + .sort_unstable_by(|a, b| a.name.cmp(&b.name)); + + // compute new indices of sorted columns + // this part won't do any check or verification. + let mut primary_key_indices = Vec::with_capacity(self.create_table.primary_keys.len()); + let mut value_indices = + Vec::with_capacity(self.create_table.column_defs.len() - primary_key_indices.len() - 1); + let mut timestamp_index = None; + for (index, col) in self.create_table.column_defs.iter().enumerate() { + if self.create_table.primary_keys.contains(&col.name) { + primary_key_indices.push(index); + } else if col.semantic_type == SemanticType::Timestamp as i32 { + timestamp_index = Some(index); + } else { + value_indices.push(index); + } + } + + // overwrite table info + self.table_info + .meta + .schema + .column_schemas + .sort_unstable_by(|a, b| a.name.cmp(&b.name)); + self.table_info.meta.schema.timestamp_index = timestamp_index; + self.table_info.meta.primary_key_indices = primary_key_indices; + self.table_info.meta.value_indices = value_indices; + } } impl Serialize for CreateTableTask { @@ -555,9 +593,11 @@ impl TryFrom for PbTruncateTableTask { mod tests { use std::sync::Arc; - use api::v1::{AlterExpr, CreateTableExpr}; - use datatypes::schema::SchemaBuilder; - use table::metadata::RawTableInfo; + use api::v1::{AlterExpr, ColumnDef, CreateTableExpr, SemanticType}; + use datatypes::schema::{ColumnSchema, RawSchema, SchemaBuilder}; + use store_api::metric_engine_consts::METRIC_ENGINE_NAME; + use store_api::storage::ConcreteDataType; + use table::metadata::{RawTableInfo, RawTableMeta, TableType}; use table::test_util::table_info::test_table_info; use super::{AlterTableTask, CreateTableTask}; @@ -589,4 +629,108 @@ mod tests { let de = serde_json::from_slice(&output).unwrap(); assert_eq!(task, de); } + + #[test] + fn test_sort_columns() { + // construct RawSchema + let raw_schema = RawSchema { + column_schemas: vec![ + ColumnSchema::new( + "column3".to_string(), + ConcreteDataType::string_datatype(), + true, + ), + ColumnSchema::new( + "column1".to_string(), + ConcreteDataType::timestamp_millisecond_datatype(), + false, + ), + ColumnSchema::new( + "column2".to_string(), + ConcreteDataType::float64_datatype(), + true, + ), + ], + timestamp_index: Some(1), + version: 0, + }; + + // construct RawTableMeta + let raw_table_meta = RawTableMeta { + schema: raw_schema, + primary_key_indices: vec![0], + value_indices: vec![2], + engine: METRIC_ENGINE_NAME.to_string(), + next_column_id: 0, + region_numbers: vec![0], + options: Default::default(), + created_on: Default::default(), + partition_key_indices: Default::default(), + }; + + // construct RawTableInfo + let raw_table_info = RawTableInfo { + ident: Default::default(), + meta: raw_table_meta, + name: Default::default(), + desc: Default::default(), + catalog_name: Default::default(), + schema_name: Default::default(), + table_type: TableType::Base, + }; + + // construct create table expr + let create_table_expr = CreateTableExpr { + column_defs: vec![ + ColumnDef { + name: "column3".to_string(), + semantic_type: SemanticType::Tag as i32, + ..Default::default() + }, + ColumnDef { + name: "column1".to_string(), + semantic_type: SemanticType::Timestamp as i32, + ..Default::default() + }, + ColumnDef { + name: "column2".to_string(), + semantic_type: SemanticType::Field as i32, + ..Default::default() + }, + ], + primary_keys: vec!["column3".to_string()], + ..Default::default() + }; + + let mut create_table_task = + CreateTableTask::new(create_table_expr, Vec::new(), raw_table_info); + + // Call the sort_columns method + create_table_task.sort_columns(); + + // Assert that the columns are sorted correctly + assert_eq!( + create_table_task.create_table.column_defs[0].name, + "column1".to_string() + ); + assert_eq!( + create_table_task.create_table.column_defs[1].name, + "column2".to_string() + ); + assert_eq!( + create_table_task.create_table.column_defs[2].name, + "column3".to_string() + ); + + // Assert that the table_info is updated correctly + assert_eq!( + create_table_task.table_info.meta.schema.timestamp_index, + Some(0) + ); + assert_eq!( + create_table_task.table_info.meta.primary_key_indices, + vec![2] + ); + assert_eq!(create_table_task.table_info.meta.value_indices, vec![1]); + } } diff --git a/src/common/meta/src/test_util.rs b/src/common/meta/src/test_util.rs index 605be9dfe3..3a312fa021 100644 --- a/src/common/meta/src/test_util.rs +++ b/src/common/meta/src/test_util.rs @@ -19,7 +19,9 @@ pub use common_base::AffectedRows; use common_recordbatch::SendableRecordBatchStream; use crate::cache_invalidator::DummyCacheInvalidator; -use crate::datanode_manager::{Datanode, DatanodeManager, DatanodeManagerRef, DatanodeRef}; +use crate::datanode_manager::{ + Datanode, DatanodeManager, DatanodeManagerRef, DatanodeRef, HandleResponse, +}; use crate::ddl::table_meta::TableMetadataAllocator; use crate::ddl::DdlContext; use crate::error::Result; @@ -32,7 +34,7 @@ use crate::wal_options_allocator::WalOptionsAllocator; #[async_trait::async_trait] pub trait MockDatanodeHandler: Sync + Send + Clone { - async fn handle(&self, peer: &Peer, request: RegionRequest) -> Result; + async fn handle(&self, peer: &Peer, request: RegionRequest) -> Result; async fn handle_query( &self, @@ -62,7 +64,7 @@ struct MockDatanode { #[async_trait::async_trait] impl Datanode for MockDatanode { - async fn handle(&self, request: RegionRequest) -> Result { + async fn handle(&self, request: RegionRequest) -> Result { self.handler.handle(&self.peer, request).await } diff --git a/src/datanode/src/region_server.rs b/src/datanode/src/region_server.rs index d9b74e02aa..917fba197d 100644 --- a/src/datanode/src/region_server.rs +++ b/src/datanode/src/region_server.rs @@ -25,6 +25,7 @@ use async_trait::async_trait; use bytes::Bytes; use common_error::ext::BoxedError; use common_error::status_code::StatusCode; +use common_meta::datanode_manager::HandleResponse; use common_query::logical_plan::Expr; use common_query::physical_plan::DfPhysicalPlanAdapter; use common_query::{DfPhysicalPlan, OutputData}; @@ -128,7 +129,7 @@ impl RegionServer { &self, region_id: RegionId, request: RegionRequest, - ) -> Result { + ) -> Result { self.inner.handle_request(region_id, request).await } @@ -267,11 +268,10 @@ impl RegionServerHandler for RegionServer { results }; - // merge results by simply sum up affected rows. - // only insert/delete will have multiple results. + // merge results by sum up affected rows and merge extensions. let mut affected_rows = 0; for result in results { - affected_rows += result; + affected_rows += result.affected_rows; } Ok(RegionResponse { @@ -282,6 +282,7 @@ impl RegionServerHandler for RegionServer { }), }), affected_rows: affected_rows as _, + extension: Default::default(), }) } } @@ -462,7 +463,7 @@ impl RegionServerInner { &self, region_id: RegionId, request: RegionRequest, - ) -> Result { + ) -> Result { let request_type = request.request_type(); let _timer = crate::metrics::HANDLE_REGION_REQUEST_ELAPSED .with_label_values(&[request_type]) @@ -487,7 +488,7 @@ impl RegionServerInner { let engine = match self.get_engine(region_id, ®ion_change)? { CurrentEngine::Engine(engine) => engine, - CurrentEngine::EarlyReturn(rows) => return Ok(rows), + CurrentEngine::EarlyReturn(rows) => return Ok(HandleResponse::new(rows)), }; // Sets corresponding region status to registering/deregistering before the operation. @@ -502,7 +503,10 @@ impl RegionServerInner { // Sets corresponding region status to ready. self.set_region_status_ready(region_id, engine, region_change) .await?; - Ok(result) + Ok(HandleResponse { + affected_rows: result.affected_rows, + extension: result.extension, + }) } Err(err) => { // Removes the region status if the operation fails. @@ -645,6 +649,7 @@ impl RegionServerInner { .decode(Bytes::from(plan), catalog_list, "", "") .await .context(DecodeLogicalPlanSnafu)?; + let result = self .query_engine .execute(logical_plan.into(), ctx) @@ -916,11 +921,11 @@ mod tests { RegionEngineWithStatus::Registering(engine.clone()), ); - let affected_rows = mock_region_server + let response = mock_region_server .handle_request(region_id, RegionRequest::Create(create_req)) .await .unwrap(); - assert_eq!(affected_rows, 0); + assert_eq!(response.affected_rows, 0); let status = mock_region_server .inner @@ -931,7 +936,7 @@ mod tests { assert!(matches!(status, RegionEngineWithStatus::Registering(_))); - let affected_rows = mock_region_server + let response = mock_region_server .handle_request( region_id, RegionRequest::Open(RegionOpenRequest { @@ -943,7 +948,7 @@ mod tests { ) .await .unwrap(); - assert_eq!(affected_rows, 0); + assert_eq!(response.affected_rows, 0); let status = mock_region_server .inner @@ -971,11 +976,11 @@ mod tests { RegionEngineWithStatus::Deregistering(engine.clone()), ); - let affected_rows = mock_region_server + let response = mock_region_server .handle_request(region_id, RegionRequest::Drop(RegionDropRequest {})) .await .unwrap(); - assert_eq!(affected_rows, 0); + assert_eq!(response.affected_rows, 0); let status = mock_region_server .inner @@ -990,11 +995,11 @@ mod tests { RegionEngineWithStatus::Deregistering(engine.clone()), ); - let affected_rows = mock_region_server + let response = mock_region_server .handle_request(region_id, RegionRequest::Close(RegionCloseRequest {})) .await .unwrap(); - assert_eq!(affected_rows, 0); + assert_eq!(response.affected_rows, 0); let status = mock_region_server .inner diff --git a/src/datanode/src/tests.rs b/src/datanode/src/tests.rs index 1a38c88f0c..cffa382865 100644 --- a/src/datanode/src/tests.rs +++ b/src/datanode/src/tests.rs @@ -31,7 +31,7 @@ use query::query_engine::DescribeResult; use query::{QueryEngine, QueryEngineContext}; use session::context::QueryContextRef; use store_api::metadata::RegionMetadataRef; -use store_api::region_engine::{RegionEngine, RegionRole, SetReadonlyResponse}; +use store_api::region_engine::{RegionEngine, RegionHandleResult, RegionRole, SetReadonlyResponse}; use store_api::region_request::{AffectedRows, RegionRequest}; use store_api::storage::{RegionId, ScanRequest}; use table::TableRef; @@ -166,16 +166,18 @@ impl RegionEngine for MockRegionEngine { &self, region_id: RegionId, request: RegionRequest, - ) -> Result { + ) -> Result { if let Some(delay) = self.handle_request_delay { tokio::time::sleep(delay).await; } if let Some(mock_fn) = &self.handle_request_mock_fn { - return mock_fn(region_id, request).map_err(BoxedError::new); + return mock_fn(region_id, request) + .map_err(BoxedError::new) + .map(RegionHandleResult::new); }; let _ = self.sender.send((region_id, request)).await; - Ok(0) + Ok(RegionHandleResult::new(0)) } async fn handle_query( diff --git a/src/datatypes/src/schema/column_schema.rs b/src/datatypes/src/schema/column_schema.rs index 17c39ee11b..f515370f0e 100644 --- a/src/datatypes/src/schema/column_schema.rs +++ b/src/datatypes/src/schema/column_schema.rs @@ -143,11 +143,22 @@ impl ColumnSchema { } /// Set the nullablity to `true` of the column. + /// Similar to [set_nullable] but take the ownership and return a owned value. + /// + /// [set_nullable]: Self::set_nullable pub fn with_nullable_set(mut self) -> Self { self.is_nullable = true; self } + /// Set the nullability to `true` of the column. + /// Similar to [with_nullable_set] but don't take the ownership + /// + /// [with_nullable_set]: Self::with_nullable_set + pub fn set_nullable(&mut self) { + self.is_nullable = true; + } + /// Creates a new [`ColumnSchema`] with given metadata. pub fn with_metadata(mut self, metadata: Metadata) -> Self { self.metadata = metadata; diff --git a/src/file-engine/src/engine.rs b/src/file-engine/src/engine.rs index f5a7d0e259..0b9192b5c9 100644 --- a/src/file-engine/src/engine.rs +++ b/src/file-engine/src/engine.rs @@ -24,7 +24,7 @@ use common_telemetry::{error, info}; use object_store::ObjectStore; use snafu::{ensure, OptionExt}; use store_api::metadata::RegionMetadataRef; -use store_api::region_engine::{RegionEngine, RegionRole, SetReadonlyResponse}; +use store_api::region_engine::{RegionEngine, RegionHandleResult, RegionRole, SetReadonlyResponse}; use store_api::region_request::{ AffectedRows, RegionCloseRequest, RegionCreateRequest, RegionDropRequest, RegionOpenRequest, RegionRequest, @@ -60,7 +60,7 @@ impl RegionEngine for FileRegionEngine { &self, region_id: RegionId, request: RegionRequest, - ) -> Result { + ) -> Result { self.inner .handle_request(region_id, request) .await @@ -154,8 +154,8 @@ impl EngineInner { &self, region_id: RegionId, request: RegionRequest, - ) -> EngineResult { - match request { + ) -> EngineResult { + let result = match request { RegionRequest::Create(req) => self.handle_create(region_id, req).await, RegionRequest::Drop(req) => self.handle_drop(region_id, req).await, RegionRequest::Open(req) => self.handle_open(region_id, req).await, @@ -164,7 +164,8 @@ impl EngineInner { operation: request.to_string(), } .fail(), - } + }; + result.map(RegionHandleResult::new) } async fn stop(&self) -> EngineResult<()> { diff --git a/src/frontend/src/instance/standalone.rs b/src/frontend/src/instance/standalone.rs index 2875850425..615e14f5d9 100644 --- a/src/frontend/src/instance/standalone.rs +++ b/src/frontend/src/instance/standalone.rs @@ -18,7 +18,7 @@ use api::v1::region::{QueryRequest, RegionRequest, RegionResponse}; use async_trait::async_trait; use client::region::check_response_header; use common_error::ext::BoxedError; -use common_meta::datanode_manager::{AffectedRows, Datanode, DatanodeManager, DatanodeRef}; +use common_meta::datanode_manager::{Datanode, DatanodeManager, DatanodeRef, HandleResponse}; use common_meta::error::{self as meta_error, Result as MetaResult}; use common_meta::peer::Peer; use common_recordbatch::SendableRecordBatchStream; @@ -63,7 +63,7 @@ impl RegionInvoker { #[async_trait] impl Datanode for RegionInvoker { - async fn handle(&self, request: RegionRequest) -> MetaResult { + async fn handle(&self, request: RegionRequest) -> MetaResult { let span = request .header .as_ref() @@ -76,10 +76,10 @@ impl Datanode for RegionInvoker { .await .map_err(BoxedError::new) .context(meta_error::ExternalSnafu)?; - check_response_header(response.header) + check_response_header(&response.header) .map_err(BoxedError::new) .context(meta_error::ExternalSnafu)?; - Ok(response.affected_rows as _) + Ok(HandleResponse::from_region_response(response)) } async fn handle_query(&self, request: QueryRequest) -> MetaResult { diff --git a/src/meta-srv/src/procedure/utils.rs b/src/meta-srv/src/procedure/utils.rs index 0a9ace054c..408aeb1f29 100644 --- a/src/meta-srv/src/procedure/utils.rs +++ b/src/meta-srv/src/procedure/utils.rs @@ -93,6 +93,7 @@ pub mod mock { }), }), affected_rows: 0, + extension: Default::default(), }) } } diff --git a/src/metric-engine/src/data_region.rs b/src/metric-engine/src/data_region.rs index 16028c7027..9207d0f107 100644 --- a/src/metric-engine/src/data_region.rs +++ b/src/metric-engine/src/data_region.rs @@ -58,18 +58,19 @@ impl DataRegion { /// Invoker don't need to set up or verify the column id. This method will adjust /// it using underlying schema. /// - /// This method will also set the nullable marker to true. + /// This method will also set the nullable marker to true. All of those change are applies + /// to `columns` in-place. pub async fn add_columns( &self, region_id: RegionId, - columns: Vec, + columns: &mut [ColumnMetadata], ) -> Result<()> { let region_id = utils::to_data_region_id(region_id); let mut retries = 0; // submit alter request while retries < MAX_RETRIES { - let request = self.assemble_alter_request(region_id, &columns).await?; + let request = self.assemble_alter_request(region_id, columns).await?; let _timer = MITO_DDL_DURATION.start_timer(); @@ -90,10 +91,12 @@ impl DataRegion { Ok(()) } + /// Generate warpped [RegionAlterRequest] with given [ColumnMetadata]. + /// This method will modify `columns` in-place. async fn assemble_alter_request( &self, region_id: RegionId, - columns: &[ColumnMetadata], + columns: &mut [ColumnMetadata], ) -> Result { // retrieve underlying version let region_metadata = self @@ -118,15 +121,14 @@ impl DataRegion { .unwrap_or(0); // overwrite semantic type - let columns = columns - .iter() + let new_columns = columns + .iter_mut() .enumerate() .map(|(delta, c)| { - let mut c = c.clone(); if c.semantic_type == SemanticType::Tag { if !c.column_schema.data_type.is_string() { return ColumnTypeMismatchSnafu { - column_type: c.column_schema.data_type, + column_type: c.column_schema.data_type.clone(), } .fail(); } @@ -138,11 +140,10 @@ impl DataRegion { }; c.column_id = new_column_id_start + delta as u32; - - c.column_schema = c.column_schema.with_nullable_set(); + c.column_schema.set_nullable(); Ok(AddColumn { - column_metadata: c, + column_metadata: c.clone(), location: None, }) }) @@ -151,7 +152,9 @@ impl DataRegion { // assemble alter request let alter_request = RegionRequest::Alter(RegionAlterRequest { schema_version: version, - kind: AlterKind::AddColumns { columns }, + kind: AlterKind::AddColumns { + columns: new_columns, + }, }); Ok(alter_request) @@ -167,6 +170,7 @@ impl DataRegion { .handle_request(region_id, RegionRequest::Put(request)) .await .context(MitoWriteOperationSnafu) + .map(|result| result.affected_rows) } pub async fn physical_columns( @@ -205,7 +209,7 @@ mod test { // TestEnv will create a logical region which changes the version to 1. assert_eq!(current_version, 1); - let new_columns = vec![ + let mut new_columns = vec![ ColumnMetadata { column_id: 0, semantic_type: SemanticType::Tag, @@ -226,7 +230,7 @@ mod test { }, ]; env.data_region() - .add_columns(env.default_physical_region_id(), new_columns) + .add_columns(env.default_physical_region_id(), &mut new_columns) .await .unwrap(); @@ -258,14 +262,14 @@ mod test { let env = TestEnv::new().await; env.init_metric_region().await; - let new_columns = vec![ColumnMetadata { + let mut new_columns = vec![ColumnMetadata { column_id: 0, semantic_type: SemanticType::Tag, column_schema: ColumnSchema::new("tag2", ConcreteDataType::int64_datatype(), false), }]; let result = env .data_region() - .add_columns(env.default_physical_region_id(), new_columns) + .add_columns(env.default_physical_region_id(), &mut new_columns) .await; assert!(result.is_err()); } diff --git a/src/metric-engine/src/engine.rs b/src/metric-engine/src/engine.rs index 1240b7cd6f..ab6991a13c 100644 --- a/src/metric-engine/src/engine.rs +++ b/src/metric-engine/src/engine.rs @@ -24,6 +24,7 @@ mod region_metadata; mod state; use std::any::Any; +use std::collections::HashMap; use std::sync::{Arc, RwLock}; use async_trait::async_trait; @@ -33,13 +34,13 @@ use common_recordbatch::SendableRecordBatchStream; use mito2::engine::MitoEngine; use store_api::metadata::RegionMetadataRef; use store_api::metric_engine_consts::METRIC_ENGINE_NAME; -use store_api::region_engine::{RegionEngine, RegionRole, SetReadonlyResponse}; -use store_api::region_request::{AffectedRows, RegionRequest}; +use store_api::region_engine::{RegionEngine, RegionHandleResult, RegionRole, SetReadonlyResponse}; +use store_api::region_request::RegionRequest; use store_api::storage::{RegionId, ScanRequest}; use self::state::MetricEngineState; use crate::data_region::DataRegion; -use crate::error::Result; +use crate::error::{Result, UnsupportedRegionRequestSnafu}; use crate::metadata_region::MetadataRegion; use crate::utils; @@ -121,23 +122,39 @@ impl RegionEngine for MetricEngine { &self, region_id: RegionId, request: RegionRequest, - ) -> Result { + ) -> Result { + let mut extension_return_value = HashMap::new(); + let result = match request { RegionRequest::Put(put) => self.inner.put_region(region_id, put).await, - RegionRequest::Delete(_) => todo!(), - RegionRequest::Create(create) => self.inner.create_region(region_id, create).await, + RegionRequest::Create(create) => { + self.inner + .create_region(region_id, create, &mut extension_return_value) + .await + } RegionRequest::Drop(drop) => self.inner.drop_region(region_id, drop).await, RegionRequest::Open(open) => self.inner.open_region(region_id, open).await, RegionRequest::Close(close) => self.inner.close_region(region_id, close).await, - RegionRequest::Alter(alter) => self.inner.alter_region(region_id, alter).await, - RegionRequest::Flush(_) => todo!(), - RegionRequest::Compact(_) => todo!(), - RegionRequest::Truncate(_) => todo!(), + RegionRequest::Alter(alter) => { + self.inner + .alter_region(region_id, alter, &mut extension_return_value) + .await + } + RegionRequest::Delete(_) + | RegionRequest::Flush(_) + | RegionRequest::Compact(_) + | RegionRequest::Truncate(_) => UnsupportedRegionRequestSnafu { request }.fail(), // It always Ok(0), all data is the latest. RegionRequest::Catchup(_) => Ok(0), }; - result.map_err(BoxedError::new) + // TODO: pass extension + result + .map_err(BoxedError::new) + .map(|rows| RegionHandleResult { + affected_rows: rows, + extension: extension_return_value, + }) } /// Handles substrait query and return a stream of record batches diff --git a/src/metric-engine/src/engine/alter.rs b/src/metric-engine/src/engine/alter.rs index 60520407ec..c99942dec6 100644 --- a/src/metric-engine/src/engine/alter.rs +++ b/src/metric-engine/src/engine/alter.rs @@ -12,13 +12,19 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::collections::HashMap; + use common_telemetry::{error, info}; -use snafu::OptionExt; +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}; +use crate::error::{ + ForbiddenPhysicalAlterSnafu, LogicalRegionNotFoundSnafu, Result, SerializeColumnMetadataSnafu, +}; use crate::metrics::FORBIDDEN_OPERATION_COUNT; use crate::utils::{to_data_region_id, to_metadata_region_id}; @@ -28,23 +34,39 @@ impl MetricEngineInner { &self, region_id: RegionId, request: RegionAlterRequest, + extension_return_value: &mut HashMap>, ) -> Result { let is_altering_physical_region = self.is_physical_region(region_id); let result = if is_altering_physical_region { self.alter_physical_region(region_id, request).await } else { - self.alter_logical_region(region_id, request).await + let physical_region_id = self.alter_logical_region(region_id, request).await?; + + // Add physical table's column to extension map. + // It's ok to overwrite existing key, as the latter come schema is more up-to-date + let physical_columns = self + .data_region + .physical_columns(physical_region_id) + .await?; + extension_return_value.insert( + ALTER_PHYSICAL_EXTENSION_KEY.to_string(), + ColumnMetadata::encode_list(&physical_columns) + .context(SerializeColumnMetadataSnafu)?, + ); + + Ok(()) }; result.map(|_| 0) } + /// Return the physical region id behind this logical region async fn alter_logical_region( &self, region_id: RegionId, request: RegionAlterRequest, - ) -> Result<()> { + ) -> Result { let physical_region_id = { let state = &self.state.read().unwrap(); state.get_physical_region_id(region_id).with_context(|| { @@ -55,7 +77,7 @@ impl MetricEngineInner { // only handle adding column let AlterKind::AddColumns { columns } = request.kind else { - return Ok(()); + return Ok(physical_region_id); }; let metadata_region_id = to_metadata_region_id(physical_region_id); @@ -92,7 +114,7 @@ impl MetricEngineInner { .await?; } - Ok(()) + Ok(physical_region_id) } async fn alter_physical_region( diff --git a/src/metric-engine/src/engine/create.rs b/src/metric-engine/src/engine/create.rs index 8fad9f9dc8..b8b39a11dd 100644 --- a/src/metric-engine/src/engine/create.rs +++ b/src/metric-engine/src/engine/create.rs @@ -15,6 +15,7 @@ use std::collections::{HashMap, HashSet}; use api::v1::SemanticType; +use common_error::ext::BoxedError; use common_telemetry::info; use common_time::Timestamp; use datatypes::data_type::ConcreteDataType; @@ -25,11 +26,12 @@ use object_store::util::join_dir; use snafu::{ensure, OptionExt, ResultExt}; use store_api::metadata::ColumnMetadata; use store_api::metric_engine_consts::{ - DATA_REGION_SUBDIR, DATA_SCHEMA_TABLE_ID_COLUMN_NAME, DATA_SCHEMA_TSID_COLUMN_NAME, - LOGICAL_TABLE_METADATA_KEY, METADATA_REGION_SUBDIR, METADATA_SCHEMA_KEY_COLUMN_INDEX, - METADATA_SCHEMA_KEY_COLUMN_NAME, METADATA_SCHEMA_TIMESTAMP_COLUMN_INDEX, - METADATA_SCHEMA_TIMESTAMP_COLUMN_NAME, METADATA_SCHEMA_VALUE_COLUMN_INDEX, - METADATA_SCHEMA_VALUE_COLUMN_NAME, PHYSICAL_TABLE_METADATA_KEY, + ALTER_PHYSICAL_EXTENSION_KEY, DATA_REGION_SUBDIR, DATA_SCHEMA_TABLE_ID_COLUMN_NAME, + DATA_SCHEMA_TSID_COLUMN_NAME, LOGICAL_TABLE_METADATA_KEY, METADATA_REGION_SUBDIR, + METADATA_SCHEMA_KEY_COLUMN_INDEX, METADATA_SCHEMA_KEY_COLUMN_NAME, + METADATA_SCHEMA_TIMESTAMP_COLUMN_INDEX, METADATA_SCHEMA_TIMESTAMP_COLUMN_NAME, + METADATA_SCHEMA_VALUE_COLUMN_INDEX, METADATA_SCHEMA_VALUE_COLUMN_NAME, + PHYSICAL_TABLE_METADATA_KEY, }; use store_api::region_engine::RegionEngine; use store_api::region_request::{AffectedRows, RegionCreateRequest, RegionRequest}; @@ -41,8 +43,9 @@ use crate::engine::options::{ }; use crate::engine::MetricEngineInner; use crate::error::{ - ConflictRegionOptionSnafu, CreateMitoRegionSnafu, InternalColumnOccupiedSnafu, - MissingRegionOptionSnafu, ParseRegionIdSnafu, PhysicalRegionNotFoundSnafu, Result, + ColumnNotFoundSnafu, ConflictRegionOptionSnafu, CreateMitoRegionSnafu, + InternalColumnOccupiedSnafu, MissingRegionOptionSnafu, MitoReadOperationSnafu, + ParseRegionIdSnafu, PhysicalRegionNotFoundSnafu, Result, SerializeColumnMetadataSnafu, }; use crate::metrics::{LOGICAL_REGION_COUNT, PHYSICAL_COLUMN_COUNT, PHYSICAL_REGION_COUNT}; use crate::utils::{to_data_region_id, to_metadata_region_id}; @@ -53,13 +56,28 @@ impl MetricEngineInner { &self, region_id: RegionId, request: RegionCreateRequest, + extension_return_value: &mut HashMap>, ) -> Result { Self::verify_region_create_request(&request)?; let result = if request.options.contains_key(PHYSICAL_TABLE_METADATA_KEY) { self.create_physical_region(region_id, request).await } else if request.options.contains_key(LOGICAL_TABLE_METADATA_KEY) { - self.create_logical_region(region_id, request).await + let physical_region_id = self.create_logical_region(region_id, request).await?; + + // Add physical table's column to extension map. + // It's ok to overwrite existing key, as the latter come schema is more up-to-date + let physical_columns = self + .data_region + .physical_columns(physical_region_id) + .await?; + extension_return_value.insert( + ALTER_PHYSICAL_EXTENSION_KEY.to_string(), + ColumnMetadata::encode_list(&physical_columns) + .context(SerializeColumnMetadataSnafu)?, + ); + + Ok(()) } else { MissingRegionOptionSnafu {}.fail() }; @@ -126,11 +144,16 @@ impl MetricEngineInner { /// This method will alter the data region to add columns if necessary. /// /// If the logical region to create already exists, this method will do nothing. + /// + /// `alter_request` is a hashmap that stores the alter requests that were executed + /// to the physical region. + /// + /// Return the physical region id of this logical region async fn create_logical_region( &self, logical_region_id: RegionId, request: RegionCreateRequest, - ) -> Result<()> { + ) -> Result { // transform IDs let physical_region_id_raw = request .options @@ -151,11 +174,12 @@ impl MetricEngineInner { .await? { info!("Create a existing logical region {logical_region_id}. Skipped"); - return Ok(()); + return Ok(data_region_id); } // find new columns to add let mut new_columns = vec![]; + let mut existing_columns = vec![]; { let state = &self.state.read().unwrap(); let physical_columns = @@ -168,6 +192,8 @@ impl MetricEngineInner { for col in &request.column_metadatas { if !physical_columns.contains(&col.column_schema.name) { new_columns.push(col.clone()); + } else { + existing_columns.push(col.column_schema.name.clone()); } } } @@ -188,9 +214,28 @@ impl MetricEngineInner { self.metadata_region .add_logical_region(metadata_region_id, logical_region_id) .await?; - for col in &request.column_metadatas { + + // register existing physical column to this new logical region. + let physical_schema = self + .data_region + .physical_columns(data_region_id) + .await + .map_err(BoxedError::new) + .context(MitoReadOperationSnafu)?; + let physical_schema_map = physical_schema + .into_iter() + .map(|metadata| (metadata.column_schema.name.clone(), metadata)) + .collect::>(); + for col in &existing_columns { + let column_metadata = physical_schema_map + .get(col) + .with_context(|| ColumnNotFoundSnafu { + name: col, + region_id: physical_region_id, + })? + .clone(); self.metadata_region - .add_column(metadata_region_id, logical_region_id, col) + .add_column(metadata_region_id, logical_region_id, &column_metadata) .await?; } @@ -203,19 +248,21 @@ impl MetricEngineInner { info!("Created new logical region {logical_region_id} on physical region {data_region_id}"); LOGICAL_REGION_COUNT.inc(); - Ok(()) + Ok(data_region_id) } + /// Execute corresponding alter requests to mito region. New added columns' [ColumnMetadata] will be + /// cloned into `added_columns`. pub(crate) async fn add_columns_to_physical_data_region( &self, data_region_id: RegionId, metadata_region_id: RegionId, logical_region_id: RegionId, - new_columns: Vec, + mut new_columns: Vec, ) -> Result<()> { // alter data region self.data_region - .add_columns(data_region_id, new_columns.clone()) + .add_columns(data_region_id, &mut new_columns) .await?; // register columns to metadata region @@ -362,13 +409,13 @@ impl MetricEngineInner { // concat region dir data_region_request.region_dir = join_dir(&request.region_dir, DATA_REGION_SUBDIR); - // convert semantic type + // change nullability for tag columns data_region_request .column_metadatas .iter_mut() .for_each(|metadata| { if metadata.semantic_type == SemanticType::Tag { - metadata.semantic_type = SemanticType::Field; + metadata.column_schema.set_nullable(); } }); diff --git a/src/metric-engine/src/engine/put.rs b/src/metric-engine/src/engine/put.rs index 5ce2bf227b..72c2aeb180 100644 --- a/src/metric-engine/src/engine/put.rs +++ b/src/metric-engine/src/engine/put.rs @@ -215,12 +215,12 @@ mod tests { // write data let logical_region_id = env.default_logical_region_id(); - let count = env + let result = env .metric() .handle_request(logical_region_id, request) .await .unwrap(); - assert_eq!(count, 5); + assert_eq!(result.affected_rows, 5); // read data from physical region let physical_region_id = env.default_physical_region_id(); @@ -287,11 +287,11 @@ mod tests { }); // write data - let count = engine + let result = engine .handle_request(logical_region_id, request) .await .unwrap(); - assert_eq!(100, count); + assert_eq!(100, result.affected_rows); } #[tokio::test] diff --git a/src/metric-engine/src/engine/read.rs b/src/metric-engine/src/engine/read.rs index 154a9a61b0..6093d41cd2 100644 --- a/src/metric-engine/src/engine/read.rs +++ b/src/metric-engine/src/engine/read.rs @@ -143,6 +143,7 @@ impl MetricEngineInner { self.default_projection(physical_region_id, logical_region_id) .await? }; + request.projection = Some(physical_projection); // add table filter @@ -186,6 +187,7 @@ impl MetricEngineInner { .get_metadata(data_region_id) .await .context(MitoReadOperationSnafu)?; + for name in projected_logical_names { // Safety: logical columns is a strict subset of physical columns physical_projection.push(physical_metadata.column_index_by_name(&name).unwrap()); @@ -301,7 +303,7 @@ mod test { .await .unwrap(); - assert_eq!(scan_req.projection.unwrap(), vec![0, 1, 4, 8, 9, 10, 11]); + assert_eq!(scan_req.projection.unwrap(), vec![11, 10, 9, 8, 0, 1, 4]); assert_eq!(scan_req.filters.len(), 1); assert_eq!( scan_req.filters[0], @@ -318,6 +320,6 @@ mod test { .transform_request(physical_region_id, logical_region_id, scan_req) .await .unwrap(); - assert_eq!(scan_req.projection.unwrap(), vec![0, 1, 4, 8, 9, 10, 11]); + assert_eq!(scan_req.projection.unwrap(), vec![11, 10, 9, 8, 0, 1, 4]); } } diff --git a/src/metric-engine/src/engine/region_metadata.rs b/src/metric-engine/src/engine/region_metadata.rs index 150d1a324c..1fa33ec39d 100644 --- a/src/metric-engine/src/engine/region_metadata.rs +++ b/src/metric-engine/src/engine/region_metadata.rs @@ -39,7 +39,8 @@ impl MetricEngineInner { .collect::>(); // sort columns on column id to ensure the order - logical_column_metadata.sort_unstable_by_key(|col| col.column_id); + logical_column_metadata + .sort_unstable_by(|c1, c2| c1.column_schema.name.cmp(&c2.column_schema.name)); Ok(logical_column_metadata) } diff --git a/src/metric-engine/src/error.rs b/src/metric-engine/src/error.rs index aa4f35472c..b256894729 100644 --- a/src/metric-engine/src/error.rs +++ b/src/metric-engine/src/error.rs @@ -19,6 +19,7 @@ use common_error::status_code::StatusCode; use common_macro::stack_trace_debug; use datatypes::prelude::ConcreteDataType; use snafu::{Location, Snafu}; +use store_api::region_request::RegionRequest; use store_api::storage::RegionId; #[derive(Snafu)] @@ -71,6 +72,13 @@ pub enum Error { location: Location, }, + #[snafu(display("Failed to serialize column metadata"))] + SerializeColumnMetadata { + #[snafu(source)] + error: serde_json::Error, + location: Location, + }, + #[snafu(display("Failed to decode base64 column value"))] DecodeColumnValue { #[snafu(source)] @@ -155,6 +163,12 @@ pub enum Error { region_id: RegionId, location: Location, }, + + #[snafu(display("Unsupported region request: {}", request))] + UnsupportedRegionRequest { + request: RegionRequest, + location: Location, + }, } pub type Result = std::result::Result; @@ -170,11 +184,14 @@ impl ErrorExt for Error { | ColumnTypeMismatch { .. } | PhysicalRegionBusy { .. } => StatusCode::InvalidArguments, - ForbiddenPhysicalAlter { .. } => StatusCode::Unsupported, + ForbiddenPhysicalAlter { .. } | UnsupportedRegionRequest { .. } => { + StatusCode::Unsupported + } MissingInternalColumn { .. } | DeserializeSemanticType { .. } | DeserializeColumnMetadata { .. } + | SerializeColumnMetadata { .. } | DecodeColumnValue { .. } | ParseRegionId { .. } | InvalidMetadata { .. } => StatusCode::Unexpected, diff --git a/src/metric-engine/src/metadata_region.rs b/src/metric-engine/src/metadata_region.rs index 918f7f1023..73ad6e9b32 100644 --- a/src/metric-engine/src/metadata_region.rs +++ b/src/metric-engine/src/metadata_region.rs @@ -167,7 +167,7 @@ impl MetadataRegion { // TODO(ruihang): avoid using `get_all` /// Get all the columns of a given logical region. - /// Return a list of (column_name, semantic_type). + /// Return a list of (column_name, column_metadata). pub async fn logical_columns( &self, physical_region_id: RegionId, diff --git a/src/mito2/src/engine.rs b/src/mito2/src/engine.rs index 16a3bca5dc..449f87dcf4 100644 --- a/src/mito2/src/engine.rs +++ b/src/mito2/src/engine.rs @@ -57,7 +57,7 @@ use object_store::manager::ObjectStoreManagerRef; use snafu::{ensure, OptionExt, ResultExt}; use store_api::logstore::LogStore; use store_api::metadata::RegionMetadataRef; -use store_api::region_engine::{RegionEngine, RegionRole, SetReadonlyResponse}; +use store_api::region_engine::{RegionEngine, RegionHandleResult, RegionRole, SetReadonlyResponse}; use store_api::region_request::{AffectedRows, RegionRequest}; use store_api::storage::{RegionId, ScanRequest}; use tokio::sync::oneshot; @@ -290,10 +290,11 @@ impl RegionEngine for MitoEngine { &self, region_id: RegionId, request: RegionRequest, - ) -> Result { + ) -> Result { self.inner .handle_request(region_id, request) .await + .map(RegionHandleResult::new) .map_err(BoxedError::new) } diff --git a/src/mito2/src/engine/basic_test.rs b/src/mito2/src/engine/basic_test.rs index ca6f0c173e..78cdb28545 100644 --- a/src/mito2/src/engine/basic_test.rs +++ b/src/mito2/src/engine/basic_test.rs @@ -111,7 +111,7 @@ async fn test_region_replay() { let engine = env.reopen_engine(engine, MitoConfig::default()).await; - let rows = engine + let result = engine .handle_request( region_id, RegionRequest::Open(RegionOpenRequest { @@ -123,7 +123,7 @@ async fn test_region_replay() { ) .await .unwrap(); - assert_eq!(0, rows); + assert_eq!(0, result.affected_rows); let request = ScanRequest::default(); let stream = engine.handle_query(region_id, request).await.unwrap(); diff --git a/src/mito2/src/engine/compaction_test.rs b/src/mito2/src/engine/compaction_test.rs index 42aba40cc0..71b859cced 100644 --- a/src/mito2/src/engine/compaction_test.rs +++ b/src/mito2/src/engine/compaction_test.rs @@ -42,7 +42,7 @@ async fn put_and_flush( }; put_rows(engine, region_id, rows).await; - let rows = engine + let result = engine .handle_request( region_id, RegionRequest::Flush(RegionFlushRequest { @@ -51,7 +51,7 @@ async fn put_and_flush( ) .await .unwrap(); - assert_eq!(0, rows); + assert_eq!(0, result.affected_rows); } async fn delete_and_flush( @@ -66,16 +66,16 @@ async fn delete_and_flush( rows: build_rows_for_key("a", rows.start, rows.end, 0), }; - let rows_affected = engine + let result = engine .handle_request( region_id, RegionRequest::Delete(RegionDeleteRequest { rows }), ) .await .unwrap(); - assert_eq!(row_cnt, rows_affected); + assert_eq!(row_cnt, result.affected_rows); - let rows = engine + let result = engine .handle_request( region_id, RegionRequest::Flush(RegionFlushRequest { @@ -84,7 +84,7 @@ async fn delete_and_flush( ) .await .unwrap(); - assert_eq!(0, rows); + assert_eq!(0, result.affected_rows); } async fn collect_stream_ts(stream: SendableRecordBatchStream) -> Vec { @@ -127,11 +127,11 @@ async fn test_compaction_region() { delete_and_flush(&engine, region_id, &column_schemas, 15..30).await; put_and_flush(&engine, region_id, &column_schemas, 15..25).await; - let output = engine + let result = engine .handle_request(region_id, RegionRequest::Compact(RegionCompactRequest {})) .await .unwrap(); - assert_eq!(output, 0); + assert_eq!(result.affected_rows, 0); let scanner = engine.scanner(region_id, ScanRequest::default()).unwrap(); assert_eq!( diff --git a/src/mito2/src/test_util.rs b/src/mito2/src/test_util.rs index 7339949490..9baa736493 100644 --- a/src/mito2/src/test_util.rs +++ b/src/mito2/src/test_util.rs @@ -712,11 +712,11 @@ pub fn delete_rows_schema(request: &RegionCreateRequest) -> Vec Vec) { - let rows = engine + let result = engine .handle_request( region_id, RegionRequest::Flush(RegionFlushRequest { row_group_size }), ) .await .unwrap(); - assert_eq!(0, rows); + assert_eq!(0, result.affected_rows); } /// Reopen a region. diff --git a/src/operator/src/delete.rs b/src/operator/src/delete.rs index 2a4737a79a..60e44cb7c6 100644 --- a/src/operator/src/delete.rs +++ b/src/operator/src/delete.rs @@ -144,7 +144,10 @@ impl Deleter { }); let results = future::try_join_all(tasks).await.context(JoinTaskSnafu)?; - let affected_rows = results.into_iter().sum::>()?; + let affected_rows = results + .into_iter() + .map(|resp| resp.map(|r| r.affected_rows)) + .sum::>()?; crate::metrics::DIST_DELETE_ROW_COUNT.inc_by(affected_rows as u64); Ok(affected_rows) } diff --git a/src/operator/src/insert.rs b/src/operator/src/insert.rs index 46fe574c38..526c540d2e 100644 --- a/src/operator/src/insert.rs +++ b/src/operator/src/insert.rs @@ -216,7 +216,10 @@ impl Inserter { }); let results = future::try_join_all(tasks).await.context(JoinTaskSnafu)?; - let affected_rows = results.into_iter().sum::>()?; + let affected_rows = results + .into_iter() + .map(|resp| resp.map(|r| r.affected_rows)) + .sum::>()?; crate::metrics::DIST_INGEST_ROW_COUNT.inc_by(affected_rows as u64); Ok(Output::new( OutputData::AffectedRows(affected_rows), diff --git a/src/operator/src/request.rs b/src/operator/src/request.rs index a300da034c..3aba4dfd70 100644 --- a/src/operator/src/request.rs +++ b/src/operator/src/request.rs @@ -181,7 +181,10 @@ impl Requester { }); let results = future::try_join_all(tasks).await.context(JoinTaskSnafu)?; - let affected_rows = results.into_iter().sum::>()?; + let affected_rows = results + .into_iter() + .map(|resp| resp.map(|r| r.affected_rows)) + .sum::>()?; Ok(affected_rows) } diff --git a/src/operator/src/statement/ddl.rs b/src/operator/src/statement/ddl.rs index 6500b99c29..ea66296462 100644 --- a/src/operator/src/statement/ddl.rs +++ b/src/operator/src/statement/ddl.rs @@ -48,6 +48,7 @@ use sql::statements::alter::AlterTable; use sql::statements::create::{CreateExternalTable, CreateTable, CreateTableLike, Partitions}; use sql::statements::sql_value_to_value; use sqlparser::ast::{Expr, Ident, Value as ParserValue}; +use store_api::metric_engine_consts::{LOGICAL_TABLE_METADATA_KEY, METRIC_ENGINE_NAME}; use table::dist_table::DistTable; use table::metadata::{self, RawTableInfo, RawTableMeta, TableId, TableInfo, TableType}; use table::requests::{AlterKind, AlterTableRequest, TableOptions}; @@ -138,6 +139,22 @@ impl StatementExecutor { partitions: Option, query_ctx: &QueryContextRef, ) -> Result { + // Check if is creating logical table + if create_table.engine == METRIC_ENGINE_NAME + && create_table + .table_options + .contains_key(LOGICAL_TABLE_METADATA_KEY) + { + return self + .create_logical_tables(&[create_table.clone()]) + .await? + .into_iter() + .next() + .context(error::UnexpectedSnafu { + violated: "expected to create a logical table", + }); + } + let _timer = crate::metrics::DIST_CREATE_TABLE.start_timer(); let schema = self .table_metadata_manager diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index 0ca9109514..1c9c970011 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -95,6 +95,16 @@ impl ColumnMetadata { column_id, }) } + + /// Encodes a vector of `ColumnMetadata` into a JSON byte vector. + pub fn encode_list(columns: &[Self]) -> serde_json::Result> { + serde_json::to_vec(columns) + } + + /// Decodes a JSON byte vector into a vector of `ColumnMetadata`. + pub fn decode_list(bytes: &[u8]) -> serde_json::Result> { + serde_json::from_slice(bytes) + } } #[cfg_attr(doc, aquamarine::aquamarine)] diff --git a/src/store-api/src/metric_engine_consts.rs b/src/store-api/src/metric_engine_consts.rs index d28e4d538a..1f0343f1b0 100644 --- a/src/store-api/src/metric_engine_consts.rs +++ b/src/store-api/src/metric_engine_consts.rs @@ -66,3 +66,7 @@ pub const PHYSICAL_TABLE_METADATA_KEY: &str = "physical_metric_table"; /// ``` /// And this key will be translated to corresponding physical **REGION** id in metasrv. pub const LOGICAL_TABLE_METADATA_KEY: &str = "on_physical_table"; + +/// HashMap key to be used in the region server's extension response. +/// Represent a list of column metadata that are added to physical table. +pub const ALTER_PHYSICAL_EXTENSION_KEY: &str = "ALTER_PHYSICAL"; diff --git a/src/store-api/src/region_engine.rs b/src/store-api/src/region_engine.rs index 3b9052a16a..8ec0d4a611 100644 --- a/src/store-api/src/region_engine.rs +++ b/src/store-api/src/region_engine.rs @@ -15,6 +15,7 @@ //! Region Engine's definition use std::any::Any; +use std::collections::HashMap; use std::fmt::Display; use std::sync::Arc; @@ -129,7 +130,7 @@ pub trait RegionEngine: Send + Sync { &self, region_id: RegionId, request: RegionRequest, - ) -> Result; + ) -> Result; /// Handles substrait query and return a stream of record batches async fn handle_query( @@ -171,3 +172,20 @@ pub trait RegionEngine: Send + Sync { } pub type RegionEngineRef = Arc; + +// TODO: reorganize the dependence to merge this struct with the +// one in common_meta +#[derive(Debug)] +pub struct RegionHandleResult { + pub affected_rows: AffectedRows, + pub extension: HashMap>, +} + +impl RegionHandleResult { + pub fn new(affected_rows: AffectedRows) -> Self { + Self { + affected_rows, + extension: Default::default(), + } + } +} diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index 628b7ce2c2..064ddbe5b7 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -567,13 +567,19 @@ impl From for TableIdent { #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub struct RawTableMeta { pub schema: RawSchema, + /// The indices of columns in primary key. Note that the index of timestamp column + /// is not included. Order matters to this array. pub primary_key_indices: Vec, + /// The indices of columns in value. Order doesn't matter to this array. pub value_indices: Vec, + /// Engine type of this table. Usually in small case. pub engine: String, + /// Deprecated. See https://github.com/GreptimeTeam/greptimedb/issues/2982 pub next_column_id: ColumnId, pub region_numbers: Vec, pub options: TableOptions, pub created_on: DateTime, + /// Order doesn't matter to this array. #[serde(default)] pub partition_key_indices: Vec, } diff --git a/tests-integration/src/prom_store.rs b/tests-integration/src/prom_store.rs index 73502c4f74..dd6d1b9b10 100644 --- a/tests-integration/src/prom_store.rs +++ b/tests-integration/src/prom_store.rs @@ -195,14 +195,14 @@ mod tests { name: prom_store::METRIC_NAME_LABEL.to_string(), value: "metric3".to_string(), }, - Label { - name: "idc".to_string(), - value: "z002".to_string(), - }, Label { name: "app".to_string(), value: "biz".to_string(), }, + Label { + name: "idc".to_string(), + value: "z002".to_string(), + }, ], timeseries.labels ); diff --git a/tests/cases/standalone/common/create/create_metric_table.result b/tests/cases/standalone/common/create/create_metric_table.result index 31a095c95a..3d4cb611cd 100644 --- a/tests/cases/standalone/common/create/create_metric_table.result +++ b/tests/cases/standalone/common/create/create_metric_table.result @@ -38,9 +38,9 @@ DESC TABLE t1; +--------+----------------------+-----+------+---------+---------------+ | Column | Type | Key | Null | Default | Semantic Type | +--------+----------------------+-----+------+---------+---------------+ +| host | String | PRI | YES | | TAG | | ts | TimestampMillisecond | PRI | NO | | TIMESTAMP | | val | Float64 | | YES | | FIELD | -| host | String | PRI | YES | | TAG | +--------+----------------------+-----+------+---------+---------------+ DESC TABLE t2; @@ -48,8 +48,8 @@ DESC TABLE t2; +--------+----------------------+-----+------+---------+---------------+ | Column | Type | Key | Null | Default | Semantic Type | +--------+----------------------+-----+------+---------+---------------+ -| ts | TimestampMillisecond | PRI | NO | | TIMESTAMP | | job | String | PRI | YES | | TAG | +| ts | TimestampMillisecond | PRI | NO | | TIMESTAMP | | val | Float64 | | YES | | FIELD | +--------+----------------------+-----+------+---------+---------------+ diff --git a/tests/cases/standalone/common/insert/logical_metric_table.result b/tests/cases/standalone/common/insert/logical_metric_table.result index 09ddac341c..390773c80f 100644 --- a/tests/cases/standalone/common/insert/logical_metric_table.result +++ b/tests/cases/standalone/common/insert/logical_metric_table.result @@ -6,18 +6,18 @@ CREATE TABLE t1 (ts timestamp time index, val double, host string primary key) e Affected Rows: 0 -INSERT INTO t1 VALUES (0, 0, 'host1'), (1, 1, 'host2'); +INSERT INTO t1 VALUES ('host1',0, 0), ('host2', 1, 1,); Affected Rows: 2 SELECT * from t1; -+-------------------------+-----+-------+ -| ts | val | host | -+-------------------------+-----+-------+ -| 1970-01-01T00:00:00.001 | 1.0 | host2 | -| 1970-01-01T00:00:00 | 0.0 | host1 | -+-------------------------+-----+-------+ ++-------+-------------------------+-----+ +| host | ts | val | ++-------+-------------------------+-----+ +| host2 | 1970-01-01T00:00:00.001 | 1.0 | +| host1 | 1970-01-01T00:00:00 | 0.0 | ++-------+-------------------------+-----+ CREATE TABLE t2 (ts timestamp time index, job string primary key, val double) engine = metric with ("on_physical_table" = "phy"); @@ -28,18 +28,18 @@ SELECT * from t2; ++ ++ -INSERT INTO t2 VALUES (0, 'job1', 0), (1, 'job2', 1); +INSERT INTO t2 VALUES ('job1', 0, 0), ('job2', 1, 1); Affected Rows: 2 SELECT * from t2; -+-------------------------+------+-----+ -| ts | job | val | -+-------------------------+------+-----+ -| 1970-01-01T00:00:00.001 | job2 | 1.0 | -| 1970-01-01T00:00:00 | job1 | 0.0 | -+-------------------------+------+-----+ ++------+-------------------------+-----+ +| job | ts | val | ++------+-------------------------+-----+ +| job2 | 1970-01-01T00:00:00.001 | 1.0 | +| job1 | 1970-01-01T00:00:00 | 0.0 | ++------+-------------------------+-----+ DROP TABLE t1; diff --git a/tests/cases/standalone/common/insert/logical_metric_table.sql b/tests/cases/standalone/common/insert/logical_metric_table.sql index fa2b6e0b6d..a7ff6adbdf 100644 --- a/tests/cases/standalone/common/insert/logical_metric_table.sql +++ b/tests/cases/standalone/common/insert/logical_metric_table.sql @@ -2,7 +2,7 @@ CREATE TABLE phy (ts timestamp time index, val double) engine=metric with ("phys CREATE TABLE t1 (ts timestamp time index, val double, host string primary key) engine = metric with ("on_physical_table" = "phy"); -INSERT INTO t1 VALUES (0, 0, 'host1'), (1, 1, 'host2'); +INSERT INTO t1 VALUES ('host1',0, 0), ('host2', 1, 1,); SELECT * from t1; @@ -10,7 +10,7 @@ CREATE TABLE t2 (ts timestamp time index, job string primary key, val double) en SELECT * from t2; -INSERT INTO t2 VALUES (0, 'job1', 0), (1, 'job2', 1); +INSERT INTO t2 VALUES ('job1', 0, 0), ('job2', 1, 1); SELECT * from t2; From 2ad0b24efad7e2616d2fe087904e4668943b352d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?x=C2=B3u=C2=B3?= Date: Mon, 25 Mar 2024 11:13:01 +0800 Subject: [PATCH 3/8] fix: set http response chartset to utf-8 when using table format (#3571) --- src/servers/src/http/table_result.rs | 2 +- src/servers/tests/http/http_handler_test.rs | 40 +++++++++++++++++++-- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/src/servers/src/http/table_result.rs b/src/servers/src/http/table_result.rs index e601213c08..a7fac46e89 100644 --- a/src/servers/src/http/table_result.rs +++ b/src/servers/src/http/table_result.rs @@ -135,7 +135,7 @@ impl IntoResponse for TableResponse { let mut resp = ( [( header::CONTENT_TYPE, - HeaderValue::from_static(mime::PLAIN.as_ref()), + HeaderValue::from_static(mime::TEXT_PLAIN_UTF_8.as_ref()), )], self.to_string(), ) diff --git a/src/servers/tests/http/http_handler_test.rs b/src/servers/tests/http/http_handler_test.rs index afcd44c262..0f0c8966ca 100644 --- a/src/servers/tests/http/http_handler_test.rs +++ b/src/servers/tests/http/http_handler_test.rs @@ -46,7 +46,7 @@ async fn test_sql_not_provided() { script_handler: None, }; - for format in ["greptimedb_v1", "influxdb_v1", "csv"] { + for format in ["greptimedb_v1", "influxdb_v1", "csv", "table"] { let query = http_handler::SqlQuery { db: None, sql: None, @@ -82,7 +82,7 @@ async fn test_sql_output_rows() { script_handler: None, }; - for format in ["greptimedb_v1", "influxdb_v1", "csv"] { + for format in ["greptimedb_v1", "influxdb_v1", "csv", "table"] { let query = create_query(format); let json = http_handler::sql( State(api_state.clone()), @@ -154,6 +154,23 @@ async fn test_sql_output_rows() { hyper::body::Bytes::from_static(b"4950\n"), ); } + HttpResponse::Table(resp) => { + use http_body::Body as HttpBody; + let mut resp = resp.into_response(); + assert_eq!( + resp.headers().get(header::CONTENT_TYPE), + Some(HeaderValue::from_static(mime::TEXT_PLAIN_UTF_8.as_ref())).as_ref(), + ); + assert_eq!( + resp.body_mut().data().await.unwrap().unwrap(), + hyper::body::Bytes::from( + r#"┌─SUM(numbers.uint32s)─┐ +│ 4950 │ +└──────────────────────┘ +"# + ), + ); + } _ => unreachable!(), } } @@ -172,7 +189,7 @@ async fn test_sql_form() { script_handler: None, }; - for format in ["greptimedb_v1", "influxdb_v1", "csv"] { + for format in ["greptimedb_v1", "influxdb_v1", "csv", "table"] { let form = create_form(format); let json = http_handler::sql( State(api_state.clone()), @@ -244,6 +261,23 @@ async fn test_sql_form() { hyper::body::Bytes::from_static(b"4950\n"), ); } + HttpResponse::Table(resp) => { + use http_body::Body as HttpBody; + let mut resp = resp.into_response(); + assert_eq!( + resp.headers().get(header::CONTENT_TYPE), + Some(HeaderValue::from_static(mime::TEXT_PLAIN_UTF_8.as_ref())).as_ref(), + ); + assert_eq!( + resp.body_mut().data().await.unwrap().unwrap(), + hyper::body::Bytes::from( + r#"┌─SUM(numbers.uint32s)─┐ +│ 4950 │ +└──────────────────────┘ +"# + ), + ); + } _ => unreachable!(), } } From 992c7ec71badf803ebfc608d9fe96e1b817a7e43 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Mon, 25 Mar 2024 11:19:30 +0800 Subject: [PATCH 4/8] feat: update physical table's schema on creating logical table (#3570) * feat: update physical table's schema on creating logical table Signed-off-by: Ruihang Xia * remove debug code Signed-off-by: Ruihang Xia * update sqlness cases Signed-off-by: Ruihang Xia * tweak ut const Signed-off-by: Ruihang Xia * update sqlness cases Signed-off-by: Ruihang Xia * invalid physical table cache Signed-off-by: Ruihang Xia --------- Signed-off-by: Ruihang Xia --- .../meta/src/ddl/create_logical_tables.rs | 134 ++++++++++++++++-- src/datanode/src/region_server.rs | 4 +- src/meta-srv/src/procedure/tests.rs | 2 +- src/metric-engine/src/engine.rs | 1 - .../common/create/create_metric_table.result | 27 ++-- .../common/create/create_metric_table.sql | 3 +- .../common/insert/logical_metric_table.result | 24 ++++ .../common/insert/logical_metric_table.sql | 4 + tests/runner/src/env.rs | 2 + 9 files changed, 179 insertions(+), 22 deletions(-) diff --git a/src/common/meta/src/ddl/create_logical_tables.rs b/src/common/meta/src/ddl/create_logical_tables.rs index 35a32142e4..0c7dd792ef 100644 --- a/src/common/meta/src/ddl/create_logical_tables.rs +++ b/src/common/meta/src/ddl/create_logical_tables.rs @@ -12,30 +12,40 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; +use std::ops::Deref; use api::v1::region::region_request::Body as PbRegionRequest; use api::v1::region::{CreateRequests, RegionRequest, RegionRequestHeader}; -use api::v1::CreateTableExpr; +use api::v1::{CreateTableExpr, SemanticType}; use async_trait::async_trait; use common_procedure::error::{FromJsonSnafu, Result as ProcedureResult, ToJsonSnafu}; use common_procedure::{Context as ProcedureContext, LockKey, Procedure, Status}; -use common_telemetry::info; use common_telemetry::tracing_context::TracingContext; +use common_telemetry::{info, warn}; use futures_util::future::join_all; use itertools::Itertools; use serde::{Deserialize, Serialize}; -use snafu::{ensure, ResultExt}; +use snafu::{ensure, OptionExt, ResultExt}; +use store_api::metadata::ColumnMetadata; +use store_api::metric_engine_consts::ALTER_PHYSICAL_EXTENSION_KEY; use store_api::storage::{RegionId, RegionNumber}; use strum::AsRefStr; use table::metadata::{RawTableInfo, TableId}; +use crate::cache_invalidator::Context; use crate::ddl::create_table_template::{build_template, CreateRequestBuilder}; use crate::ddl::utils::{add_peer_context_if_needed, handle_retry_error, region_storage_path}; use crate::ddl::DdlContext; -use crate::error::{Result, TableAlreadyExistsSnafu}; +use crate::error::{ + DecodeJsonSnafu, MetadataCorruptionSnafu, Result, TableAlreadyExistsSnafu, + TableInfoNotFoundSnafu, +}; +use crate::instruction::CacheIdent; +use crate::key::table_info::TableInfoValue; use crate::key::table_name::TableNameKey; use crate::key::table_route::TableRouteValue; +use crate::key::DeserializedValueWithBytes; use crate::lock_key::{CatalogLock, SchemaLock, TableLock, TableNameLock}; use crate::peer::Peer; use crate::rpc::ddl::CreateTableTask; @@ -169,11 +179,12 @@ impl CreateLogicalTablesProcedure { self.create_regions(region_routes).await } - /// Creates table metadata + /// Creates table metadata for logical tables and update corresponding physical + /// table's metadata. /// /// Abort(not-retry): /// - Failed to create table metadata. - pub async fn on_create_metadata(&self) -> Result { + pub async fn on_create_metadata(&mut self) -> Result { let manager = &self.context.table_metadata_manager; let physical_table_id = self.creator.data.physical_table_id(); let remaining_tasks = self.creator.data.remaining_tasks(); @@ -208,6 +219,42 @@ impl CreateLogicalTablesProcedure { .map(|task| task.table_info.ident.table_id) .collect::>(); + if !self.creator.data.physical_columns.is_empty() { + // fetch old physical table's info + let physical_table_info = self + .context + .table_metadata_manager + .get_full_table_info(self.creator.data.physical_table_id) + .await? + .0 + .context(TableInfoNotFoundSnafu { + table_name: format!("table id - {}", self.creator.data.physical_table_id), + })?; + + // generate new table info + let new_table_info = self + .creator + .data + .build_new_physical_table_info(&physical_table_info); + + // update physical table's metadata + self.context + .table_metadata_manager + .update_table_info(physical_table_info, new_table_info) + .await?; + + // invalid table cache + self.context + .cache_invalidator + .invalidate( + &Context::default(), + vec![CacheIdent::TableId(self.creator.data.physical_table_id)], + ) + .await?; + } else { + warn!("No physical columns found, leaving the physical table's schema unchanged"); + } + info!("Created {num_tables} tables {table_ids:?} metadata for physical table {physical_table_id}"); Ok(Status::done_with_output(table_ids)) @@ -275,11 +322,39 @@ impl CreateLogicalTablesProcedure { }); } - join_all(create_region_tasks) + // collect response from datanodes + let raw_schemas = join_all(create_region_tasks) .await .into_iter() + .map(|response| { + response.map(|mut response| response.extension.remove(ALTER_PHYSICAL_EXTENSION_KEY)) + }) .collect::>>()?; + if raw_schemas.is_empty() { + self.creator.data.state = CreateTablesState::CreateMetadata; + return Ok(Status::executing(false)); + } + + // verify all datanodes return the same raw schemas + // Safety: previous check ensures this vector is not empty. + let first = raw_schemas.first().unwrap(); + ensure!( + raw_schemas.iter().all(|x| x == first), + MetadataCorruptionSnafu { + err_msg: "Raw schemas from datanodes are not the same" + } + ); + + // decode raw schemas and store it + if let Some(raw_schema) = first { + let physical_columns = + ColumnMetadata::decode_list(raw_schema).context(DecodeJsonSnafu)?; + self.creator.data.physical_columns = physical_columns; + } else { + warn!("creating logical table result doesn't contains extension key `{ALTER_PHYSICAL_EXTENSION_KEY}`,leaving the physical table's schema unchanged"); + } + self.creator.data.state = CreateTablesState::CreateMetadata; // Ensures the procedures after the crash start from the `DatanodeCreateRegions` stage. @@ -357,6 +432,7 @@ impl TablesCreator { table_ids_already_exists: vec![None; len], physical_table_id, physical_region_numbers: vec![], + physical_columns: vec![], }, } } @@ -370,6 +446,7 @@ pub struct CreateTablesData { table_ids_already_exists: Vec>, physical_table_id: TableId, physical_region_numbers: Vec, + physical_columns: Vec, } impl CreateTablesData { @@ -420,6 +497,47 @@ impl CreateTablesData { }) .collect::>() } + + /// Generate the new physical table info. + /// + /// This method will consumes the physical columns. + fn build_new_physical_table_info( + &mut self, + old_table_info: &DeserializedValueWithBytes, + ) -> RawTableInfo { + let mut raw_table_info = old_table_info.deref().table_info.clone(); + + let existing_primary_key = raw_table_info + .meta + .schema + .column_schemas + .iter() + .map(|col| col.name.clone()) + .collect::>(); + let primary_key_indices = &mut raw_table_info.meta.primary_key_indices; + let value_indices = &mut raw_table_info.meta.value_indices; + value_indices.clear(); + let time_index = &mut raw_table_info.meta.schema.timestamp_index; + let columns = &mut raw_table_info.meta.schema.column_schemas; + columns.clear(); + + for (idx, col) in self.physical_columns.drain(..).enumerate() { + match col.semantic_type { + SemanticType::Tag => { + // push new primary key to the end. + if !existing_primary_key.contains(&col.column_schema.name) { + primary_key_indices.push(idx); + } + } + SemanticType::Field => value_indices.push(idx), + SemanticType::Timestamp => *time_index = Some(idx), + } + + columns.push(col.column_schema); + } + + raw_table_info + } } #[derive(Debug, Clone, Serialize, Deserialize, AsRefStr)] diff --git a/src/datanode/src/region_server.rs b/src/datanode/src/region_server.rs index 917fba197d..0dc9845850 100644 --- a/src/datanode/src/region_server.rs +++ b/src/datanode/src/region_server.rs @@ -270,8 +270,10 @@ impl RegionServerHandler for RegionServer { // merge results by sum up affected rows and merge extensions. let mut affected_rows = 0; + let mut extension = HashMap::new(); for result in results { affected_rows += result.affected_rows; + extension.extend(result.extension); } Ok(RegionResponse { @@ -282,7 +284,7 @@ impl RegionServerHandler for RegionServer { }), }), affected_rows: affected_rows as _, - extension: Default::default(), + extension, }) } } diff --git a/src/meta-srv/src/procedure/tests.rs b/src/meta-srv/src/procedure/tests.rs index 84ab373dc9..03648168e0 100644 --- a/src/meta-srv/src/procedure/tests.rs +++ b/src/meta-srv/src/procedure/tests.rs @@ -252,7 +252,7 @@ async fn test_on_datanode_create_logical_regions() { let region_routes = test_data::new_region_routes(); let datanode_manager = new_datanode_manager(®ion_server, ®ion_routes).await; let physical_table_route = TableRouteValue::physical(region_routes); - let physical_table_id = 111; + let physical_table_id = 1; let task1 = create_table_task(Some("my_table1")); let task2 = create_table_task(Some("my_table2")); diff --git a/src/metric-engine/src/engine.rs b/src/metric-engine/src/engine.rs index ab6991a13c..41f14fd766 100644 --- a/src/metric-engine/src/engine.rs +++ b/src/metric-engine/src/engine.rs @@ -148,7 +148,6 @@ impl RegionEngine for MetricEngine { RegionRequest::Catchup(_) => Ok(0), }; - // TODO: pass extension result .map_err(BoxedError::new) .map(|rows| RegionHandleResult { diff --git a/tests/cases/standalone/common/create/create_metric_table.result b/tests/cases/standalone/common/create/create_metric_table.result index 3d4cb611cd..b6578c5ca9 100644 --- a/tests/cases/standalone/common/create/create_metric_table.result +++ b/tests/cases/standalone/common/create/create_metric_table.result @@ -19,19 +19,28 @@ CREATE TABLE t2 (ts timestamp time index, job string primary key, val double) en Affected Rows: 0 -SELECT * FROM information_schema.tables WHERE engine = "metric" order by table_name; +SELECT table_catalog, table_schema, table_name, table_type, engine FROM information_schema.tables WHERE engine = 'metric' order by table_name; -Error: 3000(PlanQuery), Failed to plan SQL: No field named metric. Valid fields are information_schema.tables.table_catalog, information_schema.tables.table_schema, information_schema.tables.table_name, information_schema.tables.table_type, information_schema.tables.table_id, information_schema.tables.engine. ++---------------+--------------+------------+------------+--------+ +| table_catalog | table_schema | table_name | table_type | engine | ++---------------+--------------+------------+------------+--------+ +| greptime | public | phy | BASE TABLE | metric | +| greptime | public | t1 | BASE TABLE | metric | +| greptime | public | t2 | BASE TABLE | metric | ++---------------+--------------+------------+------------+--------+ --- We currently don't maintains physical table's schema. DESC TABLE phy; -+--------+----------------------+-----+------+---------+---------------+ -| Column | Type | Key | Null | Default | Semantic Type | -+--------+----------------------+-----+------+---------+---------------+ -| ts | TimestampMillisecond | PRI | NO | | TIMESTAMP | -| val | Float64 | | YES | | FIELD | -+--------+----------------------+-----+------+---------+---------------+ ++------------+----------------------+-----+------+---------+---------------+ +| Column | Type | Key | Null | Default | Semantic Type | ++------------+----------------------+-----+------+---------+---------------+ +| ts | TimestampMillisecond | | NO | | FIELD | +| val | Float64 | | YES | | FIELD | +| __table_id | UInt32 | PRI | NO | | TAG | +| __tsid | UInt64 | PRI | NO | | TAG | +| host | String | PRI | YES | | TAG | +| job | String | PRI | YES | | TAG | ++------------+----------------------+-----+------+---------+---------------+ DESC TABLE t1; diff --git a/tests/cases/standalone/common/create/create_metric_table.sql b/tests/cases/standalone/common/create/create_metric_table.sql index 3caf427d5a..28b3083d90 100644 --- a/tests/cases/standalone/common/create/create_metric_table.sql +++ b/tests/cases/standalone/common/create/create_metric_table.sql @@ -6,9 +6,8 @@ CREATE TABLE t1 (ts timestamp time index, val double, host string primary key) e CREATE TABLE t2 (ts timestamp time index, job string primary key, val double) engine = metric with ("on_physical_table" = "phy"); -SELECT * FROM information_schema.tables WHERE engine = "metric" order by table_name; +SELECT table_catalog, table_schema, table_name, table_type, engine FROM information_schema.tables WHERE engine = 'metric' order by table_name; --- We currently don't maintains physical table's schema. DESC TABLE phy; DESC TABLE t1; diff --git a/tests/cases/standalone/common/insert/logical_metric_table.result b/tests/cases/standalone/common/insert/logical_metric_table.result index 390773c80f..a6e958d4a0 100644 --- a/tests/cases/standalone/common/insert/logical_metric_table.result +++ b/tests/cases/standalone/common/insert/logical_metric_table.result @@ -49,6 +49,30 @@ DROP TABLE t2; Affected Rows: 0 +DESC TABLE phy; + ++------------+----------------------+-----+------+---------+---------------+ +| Column | Type | Key | Null | Default | Semantic Type | ++------------+----------------------+-----+------+---------+---------------+ +| ts | TimestampMillisecond | | NO | | FIELD | +| val | Float64 | | YES | | FIELD | +| __table_id | UInt32 | PRI | NO | | TAG | +| __tsid | UInt64 | PRI | NO | | TAG | +| host | String | PRI | YES | | TAG | +| job | String | PRI | YES | | TAG | ++------------+----------------------+-----+------+---------+---------------+ + +SELECT ts, val, __tsid, host, job FROM phy; + ++-------------------------+-----+----------------------+-------+------+ +| ts | val | __tsid | host | job | ++-------------------------+-----+----------------------+-------+------+ +| 1970-01-01T00:00:00.001 | 1.0 | 1128149335081630826 | host2 | | +| 1970-01-01T00:00:00 | 0.0 | 18067404594631612786 | host1 | | +| 1970-01-01T00:00:00.001 | 1.0 | 2176048834144407834 | | job2 | +| 1970-01-01T00:00:00 | 0.0 | 15980333303142110493 | | job1 | ++-------------------------+-----+----------------------+-------+------+ + DROP TABLE phy; Affected Rows: 0 diff --git a/tests/cases/standalone/common/insert/logical_metric_table.sql b/tests/cases/standalone/common/insert/logical_metric_table.sql index a7ff6adbdf..2157d68707 100644 --- a/tests/cases/standalone/common/insert/logical_metric_table.sql +++ b/tests/cases/standalone/common/insert/logical_metric_table.sql @@ -18,4 +18,8 @@ DROP TABLE t1; DROP TABLE t2; +DESC TABLE phy; + +SELECT ts, val, __tsid, host, job FROM phy; + DROP TABLE phy; diff --git a/tests/runner/src/env.rs b/tests/runner/src/env.rs index 0edf7e471e..ea3e3e1bc1 100644 --- a/tests/runner/src/env.rs +++ b/tests/runner/src/env.rs @@ -199,6 +199,8 @@ impl Env { }; let log_file_name = self.data_home.join(log_file_name).display().to_string(); + println!("{subcommand} log file at {log_file_name}"); + let log_file = OpenOptions::new() .create(true) .write(true) From 0f1747b80d5cc9052fcc69ed0f37852445bc7a09 Mon Sep 17 00:00:00 2001 From: tison Date: Mon, 25 Mar 2024 11:53:51 +0800 Subject: [PATCH 5/8] chore: retain original headers (#3572) Signed-off-by: tison Co-authored-by: Yingwen --- licenserc.toml | 6 ++++++ src/common/base/src/readable_size.rs | 16 +--------------- src/partition/src/lib.rs | 1 - src/partition/src/metrics.rs | 13 ------------- src/promql/src/functions/extrapolate_rate.rs | 3 ++- src/servers/src/repeated_field.rs | 19 +++---------------- 6 files changed, 12 insertions(+), 46 deletions(-) delete mode 100644 src/partition/src/metrics.rs diff --git a/licenserc.toml b/licenserc.toml index 8bcf6c4524..f01f7727e8 100644 --- a/licenserc.toml +++ b/licenserc.toml @@ -19,6 +19,12 @@ includes = [ "*.py", ] +excludes = [ + # copied sources + "src/common/base/src/readable_size.rs", + "src/servers/src/repeated_field.rs", +] + [properties] inceptionYear = 2023 copyrightOwner = "Greptime Team" diff --git a/src/common/base/src/readable_size.rs b/src/common/base/src/readable_size.rs index 8c539f3f78..21908526c7 100644 --- a/src/common/base/src/readable_size.rs +++ b/src/common/base/src/readable_size.rs @@ -1,20 +1,6 @@ // Copyright (c) 2017-present, PingCAP, Inc. Licensed under Apache-2.0. -// Copyright 2023 Greptime Team -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// This file is copied from https://github.com/tikv/raft-engine/blob/8dd2a39f359ff16f5295f35343f626e0c10132fa/src/util.rs +// This file is copied from https://github.com/tikv/raft-engine/blob/0.3.0/src/util.rs use std::fmt::{self, Debug, Display, Write}; use std::ops::{Div, Mul}; diff --git a/src/partition/src/lib.rs b/src/partition/src/lib.rs index 1d64951e10..647e7f63b8 100644 --- a/src/partition/src/lib.rs +++ b/src/partition/src/lib.rs @@ -18,7 +18,6 @@ pub mod columns; pub mod error; pub mod expr; pub mod manager; -pub mod metrics; pub mod multi_dim; pub mod partition; pub mod range; diff --git a/src/partition/src/metrics.rs b/src/partition/src/metrics.rs deleted file mode 100644 index 59f3388c48..0000000000 --- a/src/partition/src/metrics.rs +++ /dev/null @@ -1,13 +0,0 @@ -// Copyright 2023 Greptime Team -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. diff --git a/src/promql/src/functions/extrapolate_rate.rs b/src/promql/src/functions/extrapolate_rate.rs index ea0c3f2a95..7cba367145 100644 --- a/src/promql/src/functions/extrapolate_rate.rs +++ b/src/promql/src/functions/extrapolate_rate.rs @@ -13,6 +13,7 @@ // limitations under the License. // This file also contains some code from prometheus project. + // Copyright 2015 The Prometheus Authors // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -48,7 +49,7 @@ pub type Rate = ExtrapolatedRate; pub type Increase = ExtrapolatedRate; /// Part of the `extrapolatedRate` in Promql, -/// from +/// from #[derive(Debug)] pub struct ExtrapolatedRate { /// Range duration in millisecond diff --git a/src/servers/src/repeated_field.rs b/src/servers/src/repeated_field.rs index 0e3baf16a5..8346427a33 100644 --- a/src/servers/src/repeated_field.rs +++ b/src/servers/src/repeated_field.rs @@ -1,17 +1,3 @@ -// Copyright 2023 Greptime Team -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - // Copyright (c) 2019 Stepan Koltsov // // Permission is hereby granted, free of charge, to any person obtaining a copy @@ -32,8 +18,9 @@ // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE // OR OTHER DEALINGS IN THE SOFTWARE. -/// ! The [Clear] trait and [RepeatedField] are taken from [rust-protobuf](https://github.com/stepancheg/rust-protobuf/tree/master/protobuf-examples/vs-prost) -/// to leverage the pooling mechanism to avoid frequent heap allocation/deallocation when decoding deeply nested structs. +// The [Clear] trait and [RepeatedField] are taken from [rust-protobuf](https://github.com/stepancheg/rust-protobuf/tree/master/protobuf-examples/vs-prost) +// to leverage the pooling mechanism to avoid frequent heap allocation/deallocation when decoding deeply nested structs. + use std::borrow::Borrow; use std::cmp::Ordering; use std::default::Default; From bf14d339628a7bf43cf5bd8808f30fedf592f864 Mon Sep 17 00:00:00 2001 From: Weny Xu Date: Mon, 25 Mar 2024 15:12:47 +0900 Subject: [PATCH 6/8] feat: implement the drop database procedure (#3541) * refactor: remove Sync trait of Procedure * refactor: remove unnecessary async * feat: implement the drop database procedure * refactor: refactor DdlManager register_loaders * feat: register the DropDatabaseProcedureLoader * chore: fmt toml * feat: support to submit DropDatabaseTask * feat: support drop database stmt * fix: empty the tables stream * fix: ensure the factory always exists * test: update sqlness results * chore: correct comments * test: update sqlness results * test: update sqlness results * chore: apply suggestions from CR * chore: apply suggestions from CR --- Cargo.lock | 3 +- Cargo.toml | 2 +- src/catalog/src/kvbackend/manager.rs | 10 +- src/common/meta/Cargo.toml | 1 + src/common/meta/src/ddl.rs | 1 + src/common/meta/src/ddl/drop_database.rs | 171 ++++++++++++++++ .../meta/src/ddl/drop_database/cursor.rs | 141 +++++++++++++ src/common/meta/src/ddl/drop_database/end.rs | 35 ++++ .../meta/src/ddl/drop_database/executor.rs | 109 ++++++++++ .../meta/src/ddl/drop_database/metadata.rs | 43 ++++ .../meta/src/ddl/drop_database/start.rs | 65 ++++++ src/common/meta/src/ddl_manager.rs | 187 +++++++++++------- src/common/meta/src/error.rs | 13 +- src/common/meta/src/key.rs | 4 + src/common/meta/src/key/catalog_name.rs | 2 +- src/common/meta/src/key/schema_name.rs | 10 +- src/common/meta/src/key/table_name.rs | 2 +- src/common/meta/src/range_stream.rs | 2 + src/common/meta/src/rpc/ddl.rs | 69 ++++++- src/common/procedure/src/lib.rs | 6 +- src/common/procedure/src/local.rs | 8 +- src/common/procedure/src/local/runner.rs | 10 +- src/common/procedure/src/procedure.rs | 2 +- src/common/procedure/src/store.rs | 68 ++++--- src/meta-srv/src/service/admin/meta.rs | 9 +- src/operator/src/statement.rs | 13 +- src/operator/src/statement/ddl.rs | 44 +++++ .../standalone/common/catalog/schema.result | 2 +- .../common/create/create_database.result | 1 - .../common/show/show_databases_tables.result | 1 - .../common/system/information_schema.result | 21 +- 31 files changed, 904 insertions(+), 151 deletions(-) create mode 100644 src/common/meta/src/ddl/drop_database.rs create mode 100644 src/common/meta/src/ddl/drop_database/cursor.rs create mode 100644 src/common/meta/src/ddl/drop_database/end.rs create mode 100644 src/common/meta/src/ddl/drop_database/executor.rs create mode 100644 src/common/meta/src/ddl/drop_database/metadata.rs create mode 100644 src/common/meta/src/ddl/drop_database/start.rs diff --git a/Cargo.lock b/Cargo.lock index 9a12234862..5d491ee3d1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1993,6 +1993,7 @@ dependencies = [ "table", "tokio", "tonic 0.10.2", + "typetag", "uuid", ] @@ -3870,7 +3871,7 @@ checksum = "d2fabcfbdc87f4758337ca535fb41a6d701b65693ce38287d856d1674551ec9b" [[package]] name = "greptime-proto" version = "0.1.0" -source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=349cb385583697f41010dabeb3c106d58f9599b4#349cb385583697f41010dabeb3c106d58f9599b4" +source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=06f6297ff3cab578a1589741b504342fbad70453#06f6297ff3cab578a1589741b504342fbad70453" dependencies = [ "prost 0.12.3", "serde", diff --git a/Cargo.toml b/Cargo.toml index 509d7f84c8..350880e1bc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -103,7 +103,7 @@ etcd-client = "0.12" fst = "0.4.7" futures = "0.3" futures-util = "0.3" -greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "349cb385583697f41010dabeb3c106d58f9599b4" } +greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "06f6297ff3cab578a1589741b504342fbad70453" } humantime-serde = "1.1" itertools = "0.10" lazy_static = "1.4" diff --git a/src/catalog/src/kvbackend/manager.rs b/src/catalog/src/kvbackend/manager.rs index 6a6038f1da..327a081809 100644 --- a/src/catalog/src/kvbackend/manager.rs +++ b/src/catalog/src/kvbackend/manager.rs @@ -138,8 +138,7 @@ impl CatalogManager for KvBackendCatalogManager { let stream = self .table_metadata_manager .catalog_manager() - .catalog_names() - .await; + .catalog_names(); let keys = stream .try_collect::>() @@ -154,8 +153,7 @@ impl CatalogManager for KvBackendCatalogManager { let stream = self .table_metadata_manager .schema_manager() - .schema_names(catalog) - .await; + .schema_names(catalog); let mut keys = stream .try_collect::>() .await @@ -171,8 +169,7 @@ impl CatalogManager for KvBackendCatalogManager { let stream = self .table_metadata_manager .table_name_manager() - .tables(catalog, schema) - .await; + .tables(catalog, schema); let mut tables = stream .try_collect::>() .await @@ -297,7 +294,6 @@ impl CatalogManager for KvBackendCatalogManager { .table_metadata_manager .table_name_manager() .tables(catalog, schema) - .await .map_ok(|(_, v)| v.table_id()); const BATCH_SIZE: usize = 128; let user_tables = try_stream!({ diff --git a/src/common/meta/Cargo.toml b/src/common/meta/Cargo.toml index fdab5eae03..8892d602ea 100644 --- a/src/common/meta/Cargo.toml +++ b/src/common/meta/Cargo.toml @@ -53,6 +53,7 @@ strum.workspace = true table.workspace = true tokio.workspace = true tonic.workspace = true +typetag = "0.2" [dev-dependencies] chrono.workspace = true diff --git a/src/common/meta/src/ddl.rs b/src/common/meta/src/ddl.rs index d5d790f958..958d2fe187 100644 --- a/src/common/meta/src/ddl.rs +++ b/src/common/meta/src/ddl.rs @@ -32,6 +32,7 @@ pub mod alter_table; pub mod create_logical_tables; pub mod create_table; mod create_table_template; +pub mod drop_database; pub mod drop_table; pub mod table_meta; #[cfg(any(test, feature = "testing"))] diff --git a/src/common/meta/src/ddl/drop_database.rs b/src/common/meta/src/ddl/drop_database.rs new file mode 100644 index 0000000000..6454a403e3 --- /dev/null +++ b/src/common/meta/src/ddl/drop_database.rs @@ -0,0 +1,171 @@ +// Copyright 2023 Greptime Team +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +pub mod cursor; +pub mod end; +pub mod executor; +pub mod metadata; +pub mod start; +use std::fmt::Debug; + +use common_procedure::error::{Error as ProcedureError, FromJsonSnafu, ToJsonSnafu}; +use common_procedure::{ + Context as ProcedureContext, LockKey, Procedure, Result as ProcedureResult, Status, +}; +use futures::stream::BoxStream; +use serde::{Deserialize, Serialize}; +use snafu::ResultExt; +use tonic::async_trait; + +use self::start::DropDatabaseStart; +use crate::ddl::DdlContext; +use crate::error::Result; +use crate::key::table_name::TableNameValue; +use crate::lock_key::{CatalogLock, SchemaLock}; + +pub struct DropDatabaseProcedure { + /// The context of procedure runtime. + runtime_context: DdlContext, + context: DropDatabaseContext, + + state: Box, +} + +/// Target of dropping tables. +#[derive(Debug, Clone, Copy, Serialize, Deserialize)] +pub enum DropTableTarget { + Logical, + Physical, +} + +/// Context of [DropDatabaseProcedure] execution. +pub struct DropDatabaseContext { + catalog: String, + schema: String, + drop_if_exists: bool, + tables: Option>>, +} + +#[async_trait::async_trait] +#[typetag::serde(tag = "drop_database_state")] +pub(crate) trait State: Send + Debug { + /// Yields the next [State] and [Status]. + async fn next( + &mut self, + ddl_ctx: &DdlContext, + ctx: &mut DropDatabaseContext, + ) -> Result<(Box, Status)>; +} + +impl DropDatabaseProcedure { + pub const TYPE_NAME: &'static str = "metasrv-procedure::DropDatabase"; + + pub fn new(catalog: String, schema: String, drop_if_exists: bool, context: DdlContext) -> Self { + Self { + runtime_context: context, + context: DropDatabaseContext { + catalog, + schema, + drop_if_exists, + tables: None, + }, + state: Box::new(DropDatabaseStart), + } + } + + pub fn from_json(json: &str, runtime_context: DdlContext) -> ProcedureResult { + let DropDatabaseOwnedData { + catalog, + schema, + drop_if_exists, + state, + } = serde_json::from_str(json).context(FromJsonSnafu)?; + + Ok(Self { + runtime_context, + context: DropDatabaseContext { + catalog, + schema, + drop_if_exists, + tables: None, + }, + state, + }) + } +} + +#[async_trait] +impl Procedure for DropDatabaseProcedure { + fn type_name(&self) -> &str { + Self::TYPE_NAME + } + + async fn execute(&mut self, _ctx: &ProcedureContext) -> ProcedureResult { + let state = &mut self.state; + + let (next, status) = state + .next(&self.runtime_context, &mut self.context) + .await + .map_err(|e| { + if e.is_retry_later() { + ProcedureError::retry_later(e) + } else { + ProcedureError::external(e) + } + })?; + + *state = next; + Ok(status) + } + + fn dump(&self) -> ProcedureResult { + let data = DropDatabaseData { + catalog: &self.context.catalog, + schema: &self.context.schema, + drop_if_exists: self.context.drop_if_exists, + state: self.state.as_ref(), + }; + + serde_json::to_string(&data).context(ToJsonSnafu) + } + + fn lock_key(&self) -> LockKey { + let lock_key = vec![ + CatalogLock::Read(&self.context.catalog).into(), + SchemaLock::write(&self.context.catalog, &self.context.schema).into(), + ]; + + LockKey::new(lock_key) + } +} + +#[derive(Debug, Serialize)] +struct DropDatabaseData<'a> { + // The catalog name + catalog: &'a str, + // The schema name + schema: &'a str, + drop_if_exists: bool, + state: &'a dyn State, +} + +#[derive(Debug, Deserialize)] +struct DropDatabaseOwnedData { + // The catalog name + catalog: String, + // The schema name + schema: String, + drop_if_exists: bool, + state: Box, +} diff --git a/src/common/meta/src/ddl/drop_database/cursor.rs b/src/common/meta/src/ddl/drop_database/cursor.rs new file mode 100644 index 0000000000..afc5b152af --- /dev/null +++ b/src/common/meta/src/ddl/drop_database/cursor.rs @@ -0,0 +1,141 @@ +// Copyright 2023 Greptime Team +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use common_procedure::Status; +use futures::TryStreamExt; +use serde::{Deserialize, Serialize}; +use snafu::OptionExt; +use table::metadata::TableId; + +use super::executor::DropDatabaseExecutor; +use super::metadata::DropDatabaseRemoveMetadata; +use super::DropTableTarget; +use crate::ddl::drop_database::{DropDatabaseContext, State}; +use crate::ddl::DdlContext; +use crate::error::{self, Result}; +use crate::key::table_route::TableRouteValue; +use crate::key::DeserializedValueWithBytes; +use crate::table_name::TableName; + +#[derive(Debug, Serialize, Deserialize)] +pub struct DropDatabaseCursor { + target: DropTableTarget, +} + +impl DropDatabaseCursor { + /// Returns a new [DropDatabaseCursor]. + pub fn new(target: DropTableTarget) -> Self { + Self { target } + } + + fn handle_reach_end( + &mut self, + ctx: &mut DropDatabaseContext, + ) -> Result<(Box, Status)> { + match self.target { + DropTableTarget::Logical => { + // Consumes the tables stream. + ctx.tables.take(); + + Ok(( + Box::new(DropDatabaseCursor::new(DropTableTarget::Physical)), + Status::executing(true), + )) + } + DropTableTarget::Physical => Ok(( + Box::new(DropDatabaseRemoveMetadata), + Status::executing(true), + )), + } + } + + async fn handle_table( + &mut self, + ddl_ctx: &DdlContext, + ctx: &mut DropDatabaseContext, + table_name: String, + table_id: TableId, + table_route_value: DeserializedValueWithBytes, + ) -> Result<(Box, Status)> { + match (self.target, table_route_value.get_inner_ref()) { + (DropTableTarget::Logical, TableRouteValue::Logical(_)) + | (DropTableTarget::Physical, TableRouteValue::Physical(_)) => { + // TODO(weny): Maybe we can drop the table without fetching the `TableInfoValue` + let table_info_value = ddl_ctx + .table_metadata_manager + .table_info_manager() + .get(table_id) + .await? + .context(error::TableNotFoundSnafu { + table_name: &table_name, + })?; + Ok(( + Box::new(DropDatabaseExecutor::new( + TableName::new(&ctx.catalog, &ctx.schema, &table_name), + table_id, + table_info_value, + table_route_value, + self.target, + )), + Status::executing(true), + )) + } + _ => Ok(( + Box::new(DropDatabaseCursor::new(self.target)), + Status::executing(false), + )), + } + } +} + +#[async_trait::async_trait] +#[typetag::serde] +impl State for DropDatabaseCursor { + async fn next( + &mut self, + ddl_ctx: &DdlContext, + ctx: &mut DropDatabaseContext, + ) -> Result<(Box, Status)> { + if ctx.tables.as_deref().is_none() { + let tables = ddl_ctx + .table_metadata_manager + .table_name_manager() + .tables(&ctx.catalog, &ctx.schema); + ctx.tables = Some(tables); + } + // Safety: must exist + match ctx.tables.as_mut().unwrap().try_next().await? { + Some((table_name, table_name_value)) => { + let table_id = table_name_value.table_id(); + match ddl_ctx + .table_metadata_manager + .table_route_manager() + .table_route_storage() + .get_raw(table_id) + .await? + { + Some(table_route_value) => { + self.handle_table(ddl_ctx, ctx, table_name, table_id, table_route_value) + .await + } + None => Ok(( + Box::new(DropDatabaseCursor::new(self.target)), + Status::executing(false), + )), + } + } + None => self.handle_reach_end(ctx), + } + } +} diff --git a/src/common/meta/src/ddl/drop_database/end.rs b/src/common/meta/src/ddl/drop_database/end.rs new file mode 100644 index 0000000000..39e5c1a1ad --- /dev/null +++ b/src/common/meta/src/ddl/drop_database/end.rs @@ -0,0 +1,35 @@ +// Copyright 2023 Greptime Team +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use common_procedure::Status; +use serde::{Deserialize, Serialize}; + +use crate::ddl::drop_database::{DropDatabaseContext, State}; +use crate::ddl::DdlContext; +use crate::error::Result; + +#[derive(Debug, Serialize, Deserialize)] +pub struct DropDatabaseEnd; + +#[async_trait::async_trait] +#[typetag::serde] +impl State for DropDatabaseEnd { + async fn next( + &mut self, + _: &DdlContext, + _: &mut DropDatabaseContext, + ) -> Result<(Box, Status)> { + Ok((Box::new(DropDatabaseEnd), Status::done())) + } +} diff --git a/src/common/meta/src/ddl/drop_database/executor.rs b/src/common/meta/src/ddl/drop_database/executor.rs new file mode 100644 index 0000000000..096493b9ce --- /dev/null +++ b/src/common/meta/src/ddl/drop_database/executor.rs @@ -0,0 +1,109 @@ +// Copyright 2023 Greptime Team +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use common_procedure::Status; +use common_telemetry::info; +use serde::{Deserialize, Serialize}; +use snafu::OptionExt; +use table::metadata::TableId; + +use super::cursor::DropDatabaseCursor; +use super::{DropDatabaseContext, DropTableTarget}; +use crate::ddl::drop_database::State; +use crate::ddl::drop_table::executor::DropTableExecutor; +use crate::ddl::DdlContext; +use crate::error::{self, Result}; +use crate::key::table_info::TableInfoValue; +use crate::key::table_route::TableRouteValue; +use crate::key::DeserializedValueWithBytes; +use crate::region_keeper::OperatingRegionGuard; +use crate::rpc::router::operating_leader_regions; +use crate::table_name::TableName; + +#[derive(Debug, Serialize, Deserialize)] +pub struct DropDatabaseExecutor { + table_name: TableName, + table_id: TableId, + table_info_value: DeserializedValueWithBytes, + table_route_value: DeserializedValueWithBytes, + target: DropTableTarget, + #[serde(skip)] + dropping_regions: Vec, +} + +impl DropDatabaseExecutor { + /// Returns a new [DropDatabaseExecutor]. + pub fn new( + table_name: TableName, + table_id: TableId, + table_info_value: DeserializedValueWithBytes, + table_route_value: DeserializedValueWithBytes, + target: DropTableTarget, + ) -> Self { + Self { + table_name, + table_id, + table_info_value, + table_route_value, + target, + dropping_regions: vec![], + } + } +} + +impl DropDatabaseExecutor { + fn register_dropping_regions(&mut self, ddl_ctx: &DdlContext) -> Result<()> { + let region_routes = self.table_route_value.region_routes()?; + let dropping_regions = operating_leader_regions(region_routes); + let mut dropping_region_guards = Vec::with_capacity(dropping_regions.len()); + for (region_id, datanode_id) in dropping_regions { + let guard = ddl_ctx + .memory_region_keeper + .register(datanode_id, region_id) + .context(error::RegionOperatingRaceSnafu { + region_id, + peer_id: datanode_id, + })?; + dropping_region_guards.push(guard); + } + self.dropping_regions = dropping_region_guards; + Ok(()) + } +} + +#[async_trait::async_trait] +#[typetag::serde] +impl State for DropDatabaseExecutor { + async fn next( + &mut self, + ddl_ctx: &DdlContext, + _ctx: &mut DropDatabaseContext, + ) -> Result<(Box, Status)> { + self.register_dropping_regions(ddl_ctx)?; + let executor = DropTableExecutor::new(self.table_name.clone(), self.table_id, true); + executor + .on_remove_metadata(ddl_ctx, &self.table_info_value, &self.table_route_value) + .await?; + executor.invalidate_table_cache(ddl_ctx).await?; + executor + .on_drop_regions(ddl_ctx, &self.table_route_value) + .await?; + info!("Table: {}({}) is dropped", self.table_name, self.table_id); + + Ok(( + Box::new(DropDatabaseCursor::new(self.target)), + Status::executing(false), + )) + } +} diff --git a/src/common/meta/src/ddl/drop_database/metadata.rs b/src/common/meta/src/ddl/drop_database/metadata.rs new file mode 100644 index 0000000000..37129f16d5 --- /dev/null +++ b/src/common/meta/src/ddl/drop_database/metadata.rs @@ -0,0 +1,43 @@ +// Copyright 2023 Greptime Team +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use common_procedure::Status; +use serde::{Deserialize, Serialize}; + +use super::end::DropDatabaseEnd; +use crate::ddl::drop_database::{DropDatabaseContext, State}; +use crate::ddl::DdlContext; +use crate::error::Result; +use crate::key::schema_name::SchemaNameKey; + +#[derive(Debug, Serialize, Deserialize)] +pub struct DropDatabaseRemoveMetadata; + +#[async_trait::async_trait] +#[typetag::serde] +impl State for DropDatabaseRemoveMetadata { + async fn next( + &mut self, + ddl_ctx: &DdlContext, + ctx: &mut DropDatabaseContext, + ) -> Result<(Box, Status)> { + ddl_ctx + .table_metadata_manager + .schema_manager() + .delete(SchemaNameKey::new(&ctx.catalog, &ctx.schema)) + .await?; + + return Ok((Box::new(DropDatabaseEnd), Status::done())); + } +} diff --git a/src/common/meta/src/ddl/drop_database/start.rs b/src/common/meta/src/ddl/drop_database/start.rs new file mode 100644 index 0000000000..2c20517fb5 --- /dev/null +++ b/src/common/meta/src/ddl/drop_database/start.rs @@ -0,0 +1,65 @@ +// Copyright 2023 Greptime Team +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use common_procedure::Status; +use serde::{Deserialize, Serialize}; +use snafu::ensure; + +use crate::ddl::drop_database::cursor::DropDatabaseCursor; +use crate::ddl::drop_database::end::DropDatabaseEnd; +use crate::ddl::drop_database::{DropDatabaseContext, DropTableTarget, State}; +use crate::ddl::DdlContext; +use crate::error::{self, Result}; +use crate::key::schema_name::SchemaNameKey; + +#[derive(Debug, Serialize, Deserialize)] +pub struct DropDatabaseStart; + +#[async_trait::async_trait] +#[typetag::serde] +impl State for DropDatabaseStart { + /// Checks whether schema exists. + /// - Early returns if schema not exists and `drop_if_exists` is `true`. + /// - Throws an error if schema not exists and `drop_if_exists` is `false`. + async fn next( + &mut self, + ddl_ctx: &DdlContext, + ctx: &mut DropDatabaseContext, + ) -> Result<(Box, Status)> { + let exists = ddl_ctx + .table_metadata_manager + .schema_manager() + .exists(SchemaNameKey { + catalog: &ctx.catalog, + schema: &ctx.schema, + }) + .await?; + + if !exists && ctx.drop_if_exists { + return Ok((Box::new(DropDatabaseEnd), Status::done())); + } + + ensure!( + exists, + error::SchemaNotFoundSnafu { + table_schema: &ctx.schema, + } + ); + + Ok(( + Box::new(DropDatabaseCursor::new(DropTableTarget::Logical)), + Status::executing(true), + )) + } +} diff --git a/src/common/meta/src/ddl_manager.rs b/src/common/meta/src/ddl_manager.rs index 5f712fbdd8..c7ae7a23b2 100644 --- a/src/common/meta/src/ddl_manager.rs +++ b/src/common/meta/src/ddl_manager.rs @@ -14,7 +14,9 @@ use std::sync::Arc; -use common_procedure::{watcher, Output, ProcedureId, ProcedureManagerRef, ProcedureWithId}; +use common_procedure::{ + watcher, BoxedProcedureLoader, Output, ProcedureId, ProcedureManagerRef, ProcedureWithId, +}; use common_telemetry::tracing_context::{FutureExt, TracingContext}; use common_telemetry::{debug, info, tracing}; use snafu::{ensure, OptionExt, ResultExt}; @@ -25,6 +27,7 @@ use crate::datanode_manager::DatanodeManagerRef; use crate::ddl::alter_table::AlterTableProcedure; use crate::ddl::create_logical_tables::CreateLogicalTablesProcedure; use crate::ddl::create_table::CreateTableProcedure; +use crate::ddl::drop_database::DropDatabaseProcedure; use crate::ddl::drop_table::DropTableProcedure; use crate::ddl::table_meta::TableMetadataAllocatorRef; use crate::ddl::truncate_table::TruncateTableProcedure; @@ -39,12 +42,12 @@ use crate::key::table_route::TableRouteValue; use crate::key::{DeserializedValueWithBytes, TableMetadataManagerRef}; use crate::region_keeper::MemoryRegionKeeperRef; use crate::rpc::ddl::DdlTask::{ - AlterLogicalTables, AlterTable, CreateLogicalTables, CreateTable, DropLogicalTables, DropTable, - TruncateTable, + AlterLogicalTables, AlterTable, CreateLogicalTables, CreateTable, DropDatabase, + DropLogicalTables, DropTable, TruncateTable, }; use crate::rpc::ddl::{ - AlterTableTask, CreateTableTask, DropTableTask, SubmitDdlTaskRequest, SubmitDdlTaskResponse, - TruncateTableTask, + AlterTableTask, CreateTableTask, DropDatabaseTask, DropTableTask, SubmitDdlTaskRequest, + SubmitDdlTaskResponse, TruncateTableTask, }; use crate::rpc::procedure; use crate::rpc::procedure::{MigrateRegionRequest, MigrateRegionResponse, ProcedureStateResponse}; @@ -54,6 +57,8 @@ use crate::ClusterId; pub type DdlManagerRef = Arc; +pub type BoxedProcedureLoaderFactory = dyn Fn(DdlContext) -> BoxedProcedureLoader; + /// The [DdlManager] provides the ability to execute Ddl. pub struct DdlManager { procedure_manager: ProcedureManagerRef, @@ -64,8 +69,8 @@ pub struct DdlManager { memory_region_keeper: MemoryRegionKeeperRef, } +/// Returns a new [DdlManager] with all Ddl [BoxedProcedureLoader](common_procedure::procedure::BoxedProcedureLoader)s registered. impl DdlManager { - /// Returns a new [DdlManager] with all Ddl [BoxedProcedureLoader](common_procedure::procedure::BoxedProcedureLoader)s registered. pub fn try_new( procedure_manager: ProcedureManagerRef, datanode_clients: DatanodeManagerRef, @@ -103,75 +108,72 @@ impl DdlManager { } fn register_loaders(&self) -> Result<()> { - let context = self.create_context(); - - self.procedure_manager - .register_loader( + let loaders: Vec<(&str, &BoxedProcedureLoaderFactory)> = vec![ + ( CreateTableProcedure::TYPE_NAME, - Box::new(move |json| { - let context = context.clone(); - CreateTableProcedure::from_json(json, context).map(|p| Box::new(p) as _) - }), - ) - .context(RegisterProcedureLoaderSnafu { - type_name: CreateTableProcedure::TYPE_NAME, - })?; - - let context = self.create_context(); - - self.procedure_manager - .register_loader( + &|context: DdlContext| -> BoxedProcedureLoader { + Box::new(move |json: &str| { + let context = context.clone(); + CreateTableProcedure::from_json(json, context).map(|p| Box::new(p) as _) + }) + }, + ), + ( CreateLogicalTablesProcedure::TYPE_NAME, - Box::new(move |json| { - let context = context.clone(); - CreateLogicalTablesProcedure::from_json(json, context).map(|p| Box::new(p) as _) - }), - ) - .context(RegisterProcedureLoaderSnafu { - type_name: CreateLogicalTablesProcedure::TYPE_NAME, - })?; - - let context = self.create_context(); - - self.procedure_manager - .register_loader( - DropTableProcedure::TYPE_NAME, - Box::new(move |json| { - let context = context.clone(); - DropTableProcedure::from_json(json, context).map(|p| Box::new(p) as _) - }), - ) - .context(RegisterProcedureLoaderSnafu { - type_name: DropTableProcedure::TYPE_NAME, - })?; - - let context = self.create_context(); - - self.procedure_manager - .register_loader( + &|context: DdlContext| -> BoxedProcedureLoader { + Box::new(move |json: &str| { + let context = context.clone(); + CreateLogicalTablesProcedure::from_json(json, context) + .map(|p| Box::new(p) as _) + }) + }, + ), + ( AlterTableProcedure::TYPE_NAME, - Box::new(move |json| { - let context = context.clone(); - AlterTableProcedure::from_json(json, context).map(|p| Box::new(p) as _) - }), - ) - .context(RegisterProcedureLoaderSnafu { - type_name: AlterTableProcedure::TYPE_NAME, - })?; - - let context = self.create_context(); - - self.procedure_manager - .register_loader( + &|context: DdlContext| -> BoxedProcedureLoader { + Box::new(move |json: &str| { + let context = context.clone(); + AlterTableProcedure::from_json(json, context).map(|p| Box::new(p) as _) + }) + }, + ), + ( + DropTableProcedure::TYPE_NAME, + &|context: DdlContext| -> BoxedProcedureLoader { + Box::new(move |json: &str| { + let context = context.clone(); + DropTableProcedure::from_json(json, context).map(|p| Box::new(p) as _) + }) + }, + ), + ( TruncateTableProcedure::TYPE_NAME, - Box::new(move |json| { - let context = context.clone(); - TruncateTableProcedure::from_json(json, context).map(|p| Box::new(p) as _) - }), - ) - .context(RegisterProcedureLoaderSnafu { - type_name: TruncateTableProcedure::TYPE_NAME, - }) + &|context: DdlContext| -> BoxedProcedureLoader { + Box::new(move |json: &str| { + let context = context.clone(); + TruncateTableProcedure::from_json(json, context).map(|p| Box::new(p) as _) + }) + }, + ), + ( + DropDatabaseProcedure::TYPE_NAME, + &|context: DdlContext| -> BoxedProcedureLoader { + Box::new(move |json: &str| { + let context = context.clone(); + DropDatabaseProcedure::from_json(json, context).map(|p| Box::new(p) as _) + }) + }, + ), + ]; + + for (type_name, loader_factory) in loaders { + let context = self.create_context(); + self.procedure_manager + .register_loader(type_name, loader_factory(context)) + .context(RegisterProcedureLoaderSnafu { type_name })?; + } + + Ok(()) } #[tracing::instrument(skip_all)] @@ -260,6 +262,24 @@ impl DdlManager { self.submit_procedure(procedure_with_id).await } + #[tracing::instrument(skip_all)] + /// Submits and executes a drop table task. + pub async fn submit_drop_database( + &self, + _cluster_id: ClusterId, + DropDatabaseTask { + catalog, + schema, + drop_if_exists, + }: DropDatabaseTask, + ) -> Result<(ProcedureId, Option)> { + let context = self.create_context(); + let procedure = DropDatabaseProcedure::new(catalog, schema, drop_if_exists, context); + let procedure_with_id = ProcedureWithId::with_random_id(Box::new(procedure)); + + self.submit_procedure(procedure_with_id).await + } + #[tracing::instrument(skip_all)] /// Submits and executes a truncate table task. pub async fn submit_truncate_table_task( @@ -529,6 +549,28 @@ async fn handle_create_logical_table_tasks( }) } +async fn handle_drop_database_task( + ddl_manager: &DdlManager, + cluster_id: ClusterId, + drop_database_task: DropDatabaseTask, +) -> Result { + let (id, _) = ddl_manager + .submit_drop_database(cluster_id, drop_database_task.clone()) + .await?; + + let procedure_id = id.to_string(); + info!( + "Database {}.{} is dropped via procedure_id {id:?}", + drop_database_task.catalog, drop_database_task.schema + ); + + Ok(SubmitDdlTaskResponse { + key: procedure_id.into(), + table_id: None, + ..Default::default() + }) +} + /// TODO(dennis): let [`DdlManager`] implement [`ProcedureExecutor`] looks weird, find some way to refactor it. #[async_trait::async_trait] impl ProcedureExecutor for DdlManager { @@ -564,6 +606,9 @@ impl ProcedureExecutor for DdlManager { } DropLogicalTables(_) => todo!(), AlterLogicalTables(_) => todo!(), + DropDatabase(drop_database_task) => { + handle_drop_database_task(self, cluster_id, drop_database_task).await + } } } .trace(span) diff --git a/src/common/meta/src/error.rs b/src/common/meta/src/error.rs index 56823fd2e9..1e92c30fe9 100644 --- a/src/common/meta/src/error.rs +++ b/src/common/meta/src/error.rs @@ -267,6 +267,12 @@ pub enum Error { location: Location, }, + #[snafu(display("Schema nod found, schema: {}", table_schema))] + SchemaNotFound { + table_schema: String, + location: Location, + }, + #[snafu(display("Failed to rename table, reason: {}", reason))] RenameTable { reason: String, location: Location }, @@ -472,9 +478,10 @@ impl ErrorExt for Error { InvalidCatalogValue { source, .. } => source.status_code(), ConvertAlterTableRequest { source, .. } => source.status_code(), - ParseProcedureId { .. } | InvalidNumTopics { .. } | EmptyCreateTableTasks { .. } => { - StatusCode::InvalidArguments - } + ParseProcedureId { .. } + | InvalidNumTopics { .. } + | EmptyCreateTableTasks { .. } + | SchemaNotFound { .. } => StatusCode::InvalidArguments, } } diff --git a/src/common/meta/src/key.rs b/src/common/meta/src/key.rs index acc5e38c1a..225d552a0f 100644 --- a/src/common/meta/src/key.rs +++ b/src/common/meta/src/key.rs @@ -273,6 +273,10 @@ impl DeserializedValueWithByte self.inner } + pub fn get_inner_ref(&self) -> &T { + &self.inner + } + /// Returns original `bytes` pub fn get_raw_bytes(&self) -> Vec { self.bytes.to_vec() diff --git a/src/common/meta/src/key/catalog_name.rs b/src/common/meta/src/key/catalog_name.rs index fc393712ed..4bbfb367b9 100644 --- a/src/common/meta/src/key/catalog_name.rs +++ b/src/common/meta/src/key/catalog_name.rs @@ -123,7 +123,7 @@ impl CatalogManager { self.kv_backend.exists(&raw_key).await } - pub async fn catalog_names(&self) -> BoxStream<'static, Result> { + pub fn catalog_names(&self) -> BoxStream<'static, Result> { let start_key = CatalogNameKey::range_start_key(); let req = RangeRequest::new().with_prefix(start_key.as_bytes()); diff --git a/src/common/meta/src/key/schema_name.rs b/src/common/meta/src/key/schema_name.rs index 3364e1ade3..8b391ca9b3 100644 --- a/src/common/meta/src/key/schema_name.rs +++ b/src/common/meta/src/key/schema_name.rs @@ -173,8 +173,16 @@ impl SchemaManager { .transpose() } + /// Deletes a [SchemaNameKey]. + pub async fn delete(&self, schema: SchemaNameKey<'_>) -> Result<()> { + let raw_key = schema.as_raw_key(); + self.kv_backend.delete(&raw_key, false).await?; + + Ok(()) + } + /// Returns a schema stream, it lists all schemas belong to the target `catalog`. - pub async fn schema_names(&self, catalog: &str) -> BoxStream<'static, Result> { + pub fn schema_names(&self, catalog: &str) -> BoxStream<'static, Result> { let start_key = SchemaNameKey::range_start_key(catalog); let req = RangeRequest::new().with_prefix(start_key.as_bytes()); diff --git a/src/common/meta/src/key/table_name.rs b/src/common/meta/src/key/table_name.rs index 971856f2b6..75b5b86cff 100644 --- a/src/common/meta/src/key/table_name.rs +++ b/src/common/meta/src/key/table_name.rs @@ -241,7 +241,7 @@ impl TableNameManager { self.kv_backend.exists(&raw_key).await } - pub async fn tables( + pub fn tables( &self, catalog: &str, schema: &str, diff --git a/src/common/meta/src/range_stream.rs b/src/common/meta/src/range_stream.rs index ce15c5b7ec..878d5c0c9f 100644 --- a/src/common/meta/src/range_stream.rs +++ b/src/common/meta/src/range_stream.rs @@ -236,6 +236,8 @@ impl Stream for PaginationStream { PaginationStreamState::Init => { let factory = self.factory.take().expect("lost factory"); if !factory.more { + // Ensures the factory always exists. + self.factory = Some(factory); return Poll::Ready(None); } let fut = factory.read_next().boxed(); diff --git a/src/common/meta/src/rpc/ddl.rs b/src/common/meta/src/rpc/ddl.rs index b97924aa9f..f211d0b3d4 100644 --- a/src/common/meta/src/rpc/ddl.rs +++ b/src/common/meta/src/rpc/ddl.rs @@ -19,10 +19,13 @@ use api::v1::meta::{ AlterTableTask as PbAlterTableTask, AlterTableTasks as PbAlterTableTasks, CreateTableTask as PbCreateTableTask, CreateTableTasks as PbCreateTableTasks, DdlTaskRequest as PbDdlTaskRequest, DdlTaskResponse as PbDdlTaskResponse, - DropTableTask as PbDropTableTask, DropTableTasks as PbDropTableTasks, Partition, ProcedureId, + DropDatabaseTask as PbDropDatabaseTask, DropTableTask as PbDropTableTask, + DropTableTasks as PbDropTableTasks, Partition, ProcedureId, TruncateTableTask as PbTruncateTableTask, }; -use api::v1::{AlterExpr, CreateTableExpr, DropTableExpr, SemanticType, TruncateTableExpr}; +use api::v1::{ + AlterExpr, CreateTableExpr, DropDatabaseExpr, DropTableExpr, SemanticType, TruncateTableExpr, +}; use base64::engine::general_purpose; use base64::Engine as _; use prost::Message; @@ -43,6 +46,7 @@ pub enum DdlTask { CreateLogicalTables(Vec), DropLogicalTables(Vec), AlterLogicalTables(Vec), + DropDatabase(DropDatabaseTask), } impl DdlTask { @@ -79,6 +83,14 @@ impl DdlTask { }) } + pub fn new_drop_database(catalog: String, schema: String, drop_if_exists: bool) -> Self { + DdlTask::DropDatabase(DropDatabaseTask { + catalog, + schema, + drop_if_exists, + }) + } + pub fn new_alter_table(alter_table: AlterExpr) -> Self { DdlTask::AlterTable(AlterTableTask { alter_table }) } @@ -137,6 +149,9 @@ impl TryFrom for DdlTask { Ok(DdlTask::AlterLogicalTables(tasks)) } + Task::DropDatabaseTask(drop_database) => { + Ok(DdlTask::DropDatabase(drop_database.try_into()?)) + } } } } @@ -179,6 +194,7 @@ impl TryFrom for PbDdlTaskRequest { Task::AlterTableTasks(PbAlterTableTasks { tasks }) } + DdlTask::DropDatabase(task) => Task::DropDatabaseTask(task.try_into()?), }; Ok(Self { @@ -557,7 +573,7 @@ impl TryFrom for TruncateTableTask { fn try_from(pb: PbTruncateTableTask) -> Result { let truncate_table = pb.truncate_table.context(error::InvalidProtoMsgSnafu { - err_msg: "expected drop table", + err_msg: "expected truncate table", })?; Ok(Self { @@ -589,6 +605,53 @@ impl TryFrom for PbTruncateTableTask { } } +#[derive(Debug, PartialEq, Serialize, Deserialize, Clone)] +pub struct DropDatabaseTask { + pub catalog: String, + pub schema: String, + pub drop_if_exists: bool, +} + +impl TryFrom for DropDatabaseTask { + type Error = error::Error; + + fn try_from(pb: PbDropDatabaseTask) -> Result { + let DropDatabaseExpr { + catalog_name, + schema_name, + drop_if_exists, + } = pb.drop_database.context(error::InvalidProtoMsgSnafu { + err_msg: "expected drop database", + })?; + + Ok(DropDatabaseTask { + catalog: catalog_name, + schema: schema_name, + drop_if_exists, + }) + } +} + +impl TryFrom for PbDropDatabaseTask { + type Error = error::Error; + + fn try_from( + DropDatabaseTask { + catalog, + schema, + drop_if_exists, + }: DropDatabaseTask, + ) -> Result { + Ok(PbDropDatabaseTask { + drop_database: Some(DropDatabaseExpr { + catalog_name: catalog, + schema_name: schema, + drop_if_exists, + }), + }) + } +} + #[cfg(test)] mod tests { use std::sync::Arc; diff --git a/src/common/procedure/src/lib.rs b/src/common/procedure/src/lib.rs index cef90d8dfe..ece2ce4189 100644 --- a/src/common/procedure/src/lib.rs +++ b/src/common/procedure/src/lib.rs @@ -25,8 +25,8 @@ pub mod watcher; pub use crate::error::{Error, Result}; pub use crate::procedure::{ - BoxedProcedure, Context, ContextProvider, LockKey, Output, ParseIdError, Procedure, - ProcedureId, ProcedureManager, ProcedureManagerRef, ProcedureState, ProcedureWithId, Status, - StringKey, + BoxedProcedure, BoxedProcedureLoader, Context, ContextProvider, LockKey, Output, ParseIdError, + Procedure, ProcedureId, ProcedureManager, ProcedureManagerRef, ProcedureState, ProcedureWithId, + Status, StringKey, }; pub use crate::watcher::Watcher; diff --git a/src/common/procedure/src/local.rs b/src/common/procedure/src/local.rs index c68005db59..9602d0a87b 100644 --- a/src/common/procedure/src/local.rs +++ b/src/common/procedure/src/local.rs @@ -799,8 +799,10 @@ mod tests { let root_id = ProcedureId::random(); // Prepare data for the root procedure. for step in 0..3 { + let type_name = root.type_name().to_string(); + let data = root.dump().unwrap(); procedure_store - .store_procedure(root_id, step, &root, None) + .store_procedure(root_id, step, type_name, data, None) .await .unwrap(); } @@ -809,8 +811,10 @@ mod tests { let child_id = ProcedureId::random(); // Prepare data for the child procedure for step in 0..2 { + let type_name = child.type_name().to_string(); + let data = child.dump().unwrap(); procedure_store - .store_procedure(child_id, step, &child, Some(root_id)) + .store_procedure(child_id, step, type_name, data, Some(root_id)) .await .unwrap(); } diff --git a/src/common/procedure/src/local/runner.rs b/src/common/procedure/src/local/runner.rs index b787fbf77e..bd866ea1d4 100644 --- a/src/common/procedure/src/local/runner.rs +++ b/src/common/procedure/src/local/runner.rs @@ -385,7 +385,7 @@ impl Runner { } /// Extend the retry time to wait for the next retry. - async fn wait_on_err(&self, d: Duration, i: u64) { + async fn wait_on_err(&mut self, d: Duration, i: u64) { logging::info!( "Procedure {}-{} retry for the {} times after {} millis", self.procedure.type_name(), @@ -396,7 +396,7 @@ impl Runner { time::sleep(d).await; } - async fn on_suspended(&self, subprocedures: Vec) { + async fn on_suspended(&mut self, subprocedures: Vec) { let has_child = !subprocedures.is_empty(); for subprocedure in subprocedures { logging::info!( @@ -429,11 +429,15 @@ impl Runner { } async fn persist_procedure(&mut self) -> Result<()> { + let type_name = self.procedure.type_name().to_string(); + let data = self.procedure.dump()?; + self.store .store_procedure( self.meta.id, self.step, - &self.procedure, + type_name, + data, self.meta.parent_id, ) .await diff --git a/src/common/procedure/src/procedure.rs b/src/common/procedure/src/procedure.rs index b46428eb68..434d54950e 100644 --- a/src/common/procedure/src/procedure.rs +++ b/src/common/procedure/src/procedure.rs @@ -116,7 +116,7 @@ pub struct Context { /// A `Procedure` represents an operation or a set of operations to be performed step-by-step. #[async_trait] -pub trait Procedure: Send + Sync { +pub trait Procedure: Send { /// Type name of the procedure. fn type_name(&self) -> &str; diff --git a/src/common/procedure/src/store.rs b/src/common/procedure/src/store.rs index 11aeec6551..84dd39520d 100644 --- a/src/common/procedure/src/store.rs +++ b/src/common/procedure/src/store.rs @@ -22,7 +22,7 @@ use snafu::ResultExt; use crate::error::{Result, ToJsonSnafu}; pub(crate) use crate::store::state_store::StateStoreRef; -use crate::{BoxedProcedure, ProcedureId}; +use crate::ProcedureId; pub mod state_store; @@ -75,14 +75,12 @@ impl ProcedureStore { &self, procedure_id: ProcedureId, step: u32, - procedure: &BoxedProcedure, + type_name: String, + data: String, parent_id: Option, ) -> Result<()> { - let type_name = procedure.type_name(); - let data = procedure.dump()?; - let message = ProcedureMessage { - type_name: type_name.to_string(), + type_name, data, parent_id, step, @@ -312,6 +310,7 @@ mod tests { use object_store::ObjectStore; use crate::store::state_store::ObjectStateStore; + use crate::BoxedProcedure; impl ProcedureStore { pub(crate) fn from_object_store(store: ObjectStore) -> ProcedureStore { @@ -481,9 +480,10 @@ mod tests { let procedure_id = ProcedureId::random(); let procedure: BoxedProcedure = Box::new(MockProcedure::new("test store procedure")); - + let type_name = procedure.type_name().to_string(); + let data = procedure.dump().unwrap(); store - .store_procedure(procedure_id, 0, &procedure, None) + .store_procedure(procedure_id, 0, type_name, data, None) .await .unwrap(); @@ -507,9 +507,10 @@ mod tests { let procedure_id = ProcedureId::random(); let procedure: BoxedProcedure = Box::new(MockProcedure::new("test store procedure")); - + let type_name = procedure.type_name().to_string(); + let data = procedure.dump().unwrap(); store - .store_procedure(procedure_id, 0, &procedure, None) + .store_procedure(procedure_id, 0, type_name, data, None) .await .unwrap(); store.commit_procedure(procedure_id, 1).await.unwrap(); @@ -526,9 +527,10 @@ mod tests { let procedure_id = ProcedureId::random(); let procedure: BoxedProcedure = Box::new(MockProcedure::new("test store procedure")); - + let type_name = procedure.type_name().to_string(); + let data = procedure.dump().unwrap(); store - .store_procedure(procedure_id, 0, &procedure, None) + .store_procedure(procedure_id, 0, type_name, data, None) .await .unwrap(); store.rollback_procedure(procedure_id, 1).await.unwrap(); @@ -545,13 +547,16 @@ mod tests { let procedure_id = ProcedureId::random(); let procedure: BoxedProcedure = Box::new(MockProcedure::new("test store procedure")); - + let type_name = procedure.type_name().to_string(); + let data = procedure.dump().unwrap(); store - .store_procedure(procedure_id, 0, &procedure, None) + .store_procedure(procedure_id, 0, type_name, data, None) .await .unwrap(); + let type_name = procedure.type_name().to_string(); + let data = procedure.dump().unwrap(); store - .store_procedure(procedure_id, 1, &procedure, None) + .store_procedure(procedure_id, 1, type_name, data, None) .await .unwrap(); @@ -570,12 +575,17 @@ mod tests { let procedure_id = ProcedureId::random(); let procedure: BoxedProcedure = Box::new(MockProcedure::new("test store procedure")); + let type_name = procedure.type_name().to_string(); + let data = procedure.dump().unwrap(); store - .store_procedure(procedure_id, 0, &procedure, None) + .store_procedure(procedure_id, 0, type_name, data, None) .await .unwrap(); + + let type_name = procedure.type_name().to_string(); + let data = procedure.dump().unwrap(); store - .store_procedure(procedure_id, 1, &procedure, None) + .store_procedure(procedure_id, 1, type_name, data, None) .await .unwrap(); store.commit_procedure(procedure_id, 2).await.unwrap(); @@ -595,31 +605,41 @@ mod tests { // store 3 steps let id0 = ProcedureId::random(); let procedure: BoxedProcedure = Box::new(MockProcedure::new("id0-0")); + let type_name = procedure.type_name().to_string(); + let data = procedure.dump().unwrap(); store - .store_procedure(id0, 0, &procedure, None) + .store_procedure(id0, 0, type_name, data, None) .await .unwrap(); let procedure: BoxedProcedure = Box::new(MockProcedure::new("id0-1")); + let type_name = procedure.type_name().to_string(); + let data = procedure.dump().unwrap(); store - .store_procedure(id0, 1, &procedure, None) + .store_procedure(id0, 1, type_name, data, None) .await .unwrap(); let procedure: BoxedProcedure = Box::new(MockProcedure::new("id0-2")); + let type_name = procedure.type_name().to_string(); + let data = procedure.dump().unwrap(); store - .store_procedure(id0, 2, &procedure, None) + .store_procedure(id0, 2, type_name, data, None) .await .unwrap(); // store 2 steps and then commit let id1 = ProcedureId::random(); let procedure: BoxedProcedure = Box::new(MockProcedure::new("id1-0")); + let type_name = procedure.type_name().to_string(); + let data = procedure.dump().unwrap(); store - .store_procedure(id1, 0, &procedure, None) + .store_procedure(id1, 0, type_name, data, None) .await .unwrap(); let procedure: BoxedProcedure = Box::new(MockProcedure::new("id1-1")); + let type_name = procedure.type_name().to_string(); + let data = procedure.dump().unwrap(); store - .store_procedure(id1, 1, &procedure, None) + .store_procedure(id1, 1, type_name, data, None) .await .unwrap(); store.commit_procedure(id1, 2).await.unwrap(); @@ -627,8 +647,10 @@ mod tests { // store 1 step let id2 = ProcedureId::random(); let procedure: BoxedProcedure = Box::new(MockProcedure::new("id2-0")); + let type_name = procedure.type_name().to_string(); + let data = procedure.dump().unwrap(); store - .store_procedure(id2, 0, &procedure, None) + .store_procedure(id2, 0, type_name, data, None) .await .unwrap(); diff --git a/src/meta-srv/src/service/admin/meta.rs b/src/meta-srv/src/service/admin/meta.rs index 77eb38e7dd..d13ca93b0e 100644 --- a/src/meta-srv/src/service/admin/meta.rs +++ b/src/meta-srv/src/service/admin/meta.rs @@ -52,8 +52,7 @@ impl HttpHandler for CatalogsHandler { let stream = self .table_metadata_manager .catalog_manager() - .catalog_names() - .await; + .catalog_names(); let keys = stream .try_collect::>() @@ -84,8 +83,7 @@ impl HttpHandler for SchemasHandler { let stream = self .table_metadata_manager .schema_manager() - .schema_names(catalog) - .await; + .schema_names(catalog); let keys = stream .try_collect::>() @@ -118,8 +116,7 @@ impl HttpHandler for TablesHandler { let stream = self .table_metadata_manager .table_name_manager() - .tables(catalog, schema) - .await; + .tables(catalog, schema); let tables = stream .try_collect::>() .await diff --git a/src/operator/src/statement.rs b/src/operator/src/statement.rs index 974f02c52a..f7b7471ec1 100644 --- a/src/operator/src/statement.rs +++ b/src/operator/src/statement.rs @@ -168,12 +168,13 @@ impl StatementExecutor { let table_name = TableName::new(catalog, schema, table); self.drop_table(table_name, stmt.drop_if_exists()).await } - Statement::DropDatabase(_stmt) => { - // TODO(weny): implement the drop database procedure - error::NotSupportedSnafu { - feat: "Drop Database", - } - .fail() + Statement::DropDatabase(stmt) => { + self.drop_database( + query_ctx.current_catalog().to_string(), + format_raw_object_name(stmt.name()), + stmt.drop_if_exists(), + ) + .await } Statement::TruncateTable(stmt) => { let (catalog, schema, table) = diff --git a/src/operator/src/statement/ddl.rs b/src/operator/src/statement/ddl.rs index ea66296462..455885bbda 100644 --- a/src/operator/src/statement/ddl.rs +++ b/src/operator/src/statement/ddl.rs @@ -369,6 +369,34 @@ impl StatementExecutor { } } + #[tracing::instrument(skip_all)] + pub async fn drop_database( + &self, + catalog: String, + schema: String, + drop_if_exists: bool, + ) -> Result { + if self + .catalog_manager + .schema_exists(&catalog, &schema) + .await + .context(CatalogSnafu)? + { + self.drop_database_procedure(catalog, schema, drop_if_exists) + .await?; + + Ok(Output::new_with_affected_rows(0)) + } else if drop_if_exists { + // DROP TABLE IF EXISTS meets table not found - ignored + Ok(Output::new_with_affected_rows(0)) + } else { + Err(SchemaNotFoundSnafu { + schema_info: schema, + } + .into_error(snafu::NoneError)) + } + } + #[tracing::instrument(skip_all)] pub async fn truncate_table(&self, table_name: TableName) -> Result { let table = self @@ -545,6 +573,22 @@ impl StatementExecutor { .context(error::ExecuteDdlSnafu) } + async fn drop_database_procedure( + &self, + catalog: String, + schema: String, + drop_if_exists: bool, + ) -> Result { + let request = SubmitDdlTaskRequest { + task: DdlTask::new_drop_database(catalog, schema, drop_if_exists), + }; + + self.procedure_executor + .submit_ddl_task(&ExecutorContext::default(), request) + .await + .context(error::ExecuteDdlSnafu) + } + async fn truncate_table_procedure( &self, table_name: &TableName, diff --git a/tests/cases/standalone/common/catalog/schema.result b/tests/cases/standalone/common/catalog/schema.result index 4c0a29be1f..600415d312 100644 --- a/tests/cases/standalone/common/catalog/schema.result +++ b/tests/cases/standalone/common/catalog/schema.result @@ -120,7 +120,7 @@ SHOW TABLES FROM public WHERE Tables = 'numbers'; DROP SCHEMA test_public_schema; -Error: 1001(Unsupported), Not supported: Drop Database +Affected Rows: 0 SELECT * FROM test_public_schema.hello; diff --git a/tests/cases/standalone/common/create/create_database.result b/tests/cases/standalone/common/create/create_database.result index 780aca34c3..da409471c2 100644 --- a/tests/cases/standalone/common/create/create_database.result +++ b/tests/cases/standalone/common/create/create_database.result @@ -19,6 +19,5 @@ show databases; | illegal-database | | information_schema | | public | -| test_public_schema | +--------------------+ diff --git a/tests/cases/standalone/common/show/show_databases_tables.result b/tests/cases/standalone/common/show/show_databases_tables.result index 0417ffb85d..ca5fb1df9d 100644 --- a/tests/cases/standalone/common/show/show_databases_tables.result +++ b/tests/cases/standalone/common/show/show_databases_tables.result @@ -7,7 +7,6 @@ show databases; | illegal-database | | information_schema | | public | -| test_public_schema | | upper_case_table_name | +-----------------------+ diff --git a/tests/cases/standalone/common/system/information_schema.result b/tests/cases/standalone/common/system/information_schema.result index 23764d8c2b..a6dec1d3ba 100644 --- a/tests/cases/standalone/common/system/information_schema.result +++ b/tests/cases/standalone/common/system/information_schema.result @@ -447,7 +447,7 @@ Affected Rows: 0 drop schema my_db; -Error: 1001(Unsupported), Not supported: Drop Database +Affected Rows: 0 use information_schema; @@ -456,11 +456,8 @@ Affected Rows: 0 -- test query filter for key_column_usage -- select * from KEY_COLUMN_USAGE where CONSTRAINT_NAME = 'TIME INDEX'; -+--------------------+-------------------+-----------------+---------------+--------------+------------+-------------+------------------+-------------------------------+-------------------------+-----------------------+------------------------+ -| constraint_catalog | constraint_schema | constraint_name | table_catalog | table_schema | table_name | column_name | ordinal_position | position_in_unique_constraint | referenced_table_schema | referenced_table_name | referenced_column_name | -+--------------------+-------------------+-----------------+---------------+--------------+------------+-------------+------------------+-------------------------------+-------------------------+-----------------------+------------------------+ -| def | my_db | TIME INDEX | def | my_db | foo | ts | 1 | | | | | -+--------------------+-------------------+-----------------+---------------+--------------+------------+-------------+------------------+-------------------------------+-------------------------+-----------------------+------------------------+ +++ +++ select * from KEY_COLUMN_USAGE where CONSTRAINT_NAME != 'TIME INDEX'; @@ -472,11 +469,8 @@ select * from KEY_COLUMN_USAGE where CONSTRAINT_NAME != 'TIME INDEX'; select * from KEY_COLUMN_USAGE where CONSTRAINT_NAME LIKE '%INDEX'; -+--------------------+-------------------+-----------------+---------------+--------------+------------+-------------+------------------+-------------------------------+-------------------------+-----------------------+------------------------+ -| constraint_catalog | constraint_schema | constraint_name | table_catalog | table_schema | table_name | column_name | ordinal_position | position_in_unique_constraint | referenced_table_schema | referenced_table_name | referenced_column_name | -+--------------------+-------------------+-----------------+---------------+--------------+------------+-------------+------------------+-------------------------------+-------------------------+-----------------------+------------------------+ -| def | my_db | TIME INDEX | def | my_db | foo | ts | 1 | | | | | -+--------------------+-------------------+-----------------+---------------+--------------+------------+-------------+------------------+-------------------------------+-------------------------+-----------------------+------------------------+ +++ +++ select * from KEY_COLUMN_USAGE where CONSTRAINT_NAME NOT LIKE '%INDEX'; @@ -512,8 +506,6 @@ select * from schemata where catalog_name = 'greptime' and schema_name != 'publi | greptime | greptime_private | utf8 | utf8_bin | | | greptime | illegal-database | utf8 | utf8_bin | | | greptime | information_schema | utf8 | utf8_bin | | -| greptime | my_db | utf8 | utf8_bin | | -| greptime | test_public_schema | utf8 | utf8_bin | | | greptime | upper_case_table_name | utf8 | utf8_bin | | +--------------+-----------------------+----------------------------+------------------------+----------+ @@ -570,7 +562,6 @@ select * from key_column_usage; +--------------------+-------------------+-----------------+---------------+--------------+------------+-------------+------------------+-------------------------------+-------------------------+-----------------------+------------------------+ | constraint_catalog | constraint_schema | constraint_name | table_catalog | table_schema | table_name | column_name | ordinal_position | position_in_unique_constraint | referenced_table_schema | referenced_table_name | referenced_column_name | +--------------------+-------------------+-----------------+---------------+--------------+------------+-------------+------------------+-------------------------------+-------------------------+-----------------------+------------------------+ -| def | my_db | TIME INDEX | def | my_db | foo | ts | 1 | | | | | | def | public | PRIMARY | def | public | numbers | number | 1 | | | | | +--------------------+-------------------+-----------------+---------------+--------------+------------+-------------+------------------+-------------------------------+-------------------------+-----------------------+------------------------+ @@ -684,7 +675,7 @@ DESC TABLE GREPTIME_REGION_PEERS; drop table my_db.foo; -Affected Rows: 0 +Error: 4001(TableNotFound), Table not found: greptime.my_db.foo use public; From 62d8bbb10c972ea3109d6268ab1671728363a5cc Mon Sep 17 00:00:00 2001 From: Yingwen Date: Tue, 26 Mar 2024 12:04:57 +0800 Subject: [PATCH 7/8] ci: use single commit on the deployment branch (#3580) --- .github/workflows/apidoc.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/apidoc.yml b/.github/workflows/apidoc.yml index 06e845f630..ca1befa52c 100644 --- a/.github/workflows/apidoc.yml +++ b/.github/workflows/apidoc.yml @@ -40,3 +40,4 @@ jobs: uses: JamesIves/github-pages-deploy-action@v4 with: folder: target/doc + single-commit: true From 7c1c6e8b8c66c28e46a56f22b47660722520b555 Mon Sep 17 00:00:00 2001 From: tison Date: Tue, 26 Mar 2024 12:28:14 +0800 Subject: [PATCH 8/8] refactor: try upgrade regex-automata (#3575) * refactor: try upgrade regex-automata Signed-off-by: tison * try fix Signed-off-by: tison * always check match with next_eoi_state Signed-off-by: tison * add a guard to prevent over moving the state Signed-off-by: tison * tidy Signed-off-by: tison --------- Signed-off-by: tison --- Cargo.lock | 13 +--- Cargo.toml | 2 +- src/index/src/inverted_index/error.rs | 2 +- .../search/fst_apply/intersection_apply.rs | 70 +++++++++++++++---- 4 files changed, 61 insertions(+), 26 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5d491ee3d1..85909a5f3f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4378,7 +4378,7 @@ dependencies = [ "prost 0.12.3", "rand", "regex", - "regex-automata 0.2.0", + "regex-automata 0.4.3", "snafu", "tempfile", "tokio", @@ -7801,17 +7801,6 @@ dependencies = [ "regex-syntax 0.6.29", ] -[[package]] -name = "regex-automata" -version = "0.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e9368763f5a9b804326f3af749e16f9abf378d227bcdee7634b13d8f17793782" -dependencies = [ - "fst", - "memchr", - "regex-syntax 0.6.29", -] - [[package]] name = "regex-automata" version = "0.4.3" diff --git a/Cargo.toml b/Cargo.toml index 350880e1bc..cebad1ef89 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -125,7 +125,7 @@ prost = "0.12" raft-engine = { version = "0.4.1", default-features = false } rand = "0.8" regex = "1.8" -regex-automata = { version = "0.2", features = ["transducer"] } +regex-automata = { version = "0.4" } reqwest = { version = "0.11", default-features = false, features = [ "json", "rustls-tls-native-roots", diff --git a/src/index/src/inverted_index/error.rs b/src/index/src/inverted_index/error.rs index c3c40dddea..d831eaa1b1 100644 --- a/src/index/src/inverted_index/error.rs +++ b/src/index/src/inverted_index/error.rs @@ -113,7 +113,7 @@ pub enum Error { #[snafu(display("Failed to parse regex DFA"))] ParseDFA { #[snafu(source)] - error: Box, + error: Box, location: Location, }, diff --git a/src/index/src/inverted_index/search/fst_apply/intersection_apply.rs b/src/index/src/inverted_index/search/fst_apply/intersection_apply.rs index d76b44fe9d..ed8d483230 100644 --- a/src/index/src/inverted_index/search/fst_apply/intersection_apply.rs +++ b/src/index/src/inverted_index/search/fst_apply/intersection_apply.rs @@ -17,6 +17,10 @@ use std::mem::size_of; use fst::map::OpBuilder; use fst::{IntoStreamer, Streamer}; use regex_automata::dfa::dense::DFA; +use regex_automata::dfa::Automaton; +use regex_automata::util::primitives::StateID; +use regex_automata::util::start::Config; +use regex_automata::Anchored; use snafu::{ensure, ResultExt}; use crate::inverted_index::error::{ @@ -32,7 +36,53 @@ pub struct IntersectionFstApplier { ranges: Vec, /// A list of `Dfa` compiled from regular expression patterns. - dfas: Vec>>, + dfas: Vec, +} + +#[derive(Debug)] +struct DfaFstAutomaton(DFA>); + +impl fst::Automaton for DfaFstAutomaton { + type State = StateID; + + #[inline] + fn start(&self) -> Self::State { + let config = Config::new().anchored(Anchored::No); + self.0.start_state(&config).unwrap() + } + + #[inline] + fn is_match(&self, state: &Self::State) -> bool { + self.0.is_match_state(*state) + } + + #[inline] + fn can_match(&self, state: &Self::State) -> bool { + !self.0.is_dead_state(*state) + } + + #[inline] + fn accept_eof(&self, state: &StateID) -> Option { + if self.0.is_match_state(*state) { + return Some(*state); + } + Some(self.0.next_eoi_state(*state)) + } + + #[inline] + fn accept(&self, state: &Self::State, byte: u8) -> Self::State { + if self.0.is_match_state(*state) { + return *state; + } + self.0.next_state(*state, byte) + } +} + +impl IntersectionFstApplier { + fn new(ranges: Vec, dfas: Vec>>) -> Self { + let dfas = dfas.into_iter().map(DfaFstAutomaton).collect(); + Self { ranges, dfas } + } } impl FstApplier for IntersectionFstApplier { @@ -86,7 +136,7 @@ impl FstApplier for IntersectionFstApplier { size += self.dfas.capacity() * size_of::>>(); for dfa in &self.dfas { - size += dfa.memory_usage(); + size += dfa.0.memory_usage(); } size } @@ -119,7 +169,7 @@ impl IntersectionFstApplier { } } - Ok(Self { dfas, ranges }) + Ok(Self::new(ranges, dfas)) } } @@ -365,18 +415,15 @@ mod tests { #[test] fn test_intersection_fst_applier_memory_usage() { - let applier = IntersectionFstApplier { - ranges: vec![], - dfas: vec![], - }; + let applier = IntersectionFstApplier::new(vec![], vec![]); assert_eq!(applier.memory_usage(), 0); let dfa = DFA::new("^abc$").unwrap(); assert_eq!(dfa.memory_usage(), 320); - let applier = IntersectionFstApplier { - ranges: vec![Range { + let applier = IntersectionFstApplier::new( + vec![Range { lower: Some(Bound { value: b"aa".to_vec(), inclusive: true, @@ -386,9 +433,8 @@ mod tests { inclusive: true, }), }], - dfas: vec![dfa], - }; - + vec![dfa], + ); assert_eq!( applier.memory_usage(), size_of::() + 4 + size_of::>>() + 320