diff --git a/src/query/service/src/interpreters/interpreter_table_drop.rs b/src/query/service/src/interpreters/interpreter_table_drop.rs index 1321cf6d1daf..f33e1e94452b 100644 --- a/src/query/service/src/interpreters/interpreter_table_drop.rs +++ b/src/query/service/src/interpreters/interpreter_table_drop.rs @@ -54,94 +54,97 @@ impl Interpreter for DropTableInterpreter { let catalog_name = self.plan.catalog.as_str(); let db_name = self.plan.database.as_str(); let tbl_name = self.plan.table.as_str(); - let tbl = self - .ctx - .get_table(catalog_name, db_name, tbl_name) - .await - .ok(); + let tbl = match self.ctx.get_table(catalog_name, db_name, tbl_name).await { + Ok(table) => table, + Err(error) => { + if (error.code() == ErrorCode::UNKNOWN_TABLE + || error.code() == ErrorCode::UNKNOWN_CATALOG + || error.code() == ErrorCode::UNKNOWN_DATABASE) + && self.plan.if_exists + { + return Ok(PipelineBuildResult::create()); + } else { + return Err(error); + } + } + }; - if tbl.is_none() && !self.plan.if_exists { - return Err(ErrorCode::UnknownTable(format!( - "Unknown table `{}`.`{}` in catalog '{}'", - db_name, tbl_name, catalog_name + let engine = tbl.get_table_info().engine(); + if matches!(engine, VIEW_ENGINE | STREAM_ENGINE) { + return Err(ErrorCode::TableEngineNotSupported(format!( + "{}.{} engine is {} that doesn't support drop, use `DROP {} {}.{}` instead", + &self.plan.database, + &self.plan.table, + engine, + engine, + &self.plan.database, + &self.plan.table ))); } - if let Some(tbl) = tbl { - let engine = tbl.get_table_info().engine(); - if matches!(engine, VIEW_ENGINE | STREAM_ENGINE) { - return Err(ErrorCode::TableEngineNotSupported(format!( - "{}.{} engine is {} that doesn't support drop, use `DROP {} {}.{}` instead", - &self.plan.database, - &self.plan.table, - engine, - engine, - &self.plan.database, - &self.plan.table - ))); - } - let catalog = self.ctx.get_catalog(catalog_name).await?; - - // drop the ownership - let tenant = self.ctx.get_tenant(); - let role_api = UserApiProvider::instance().get_role_api_client(&self.plan.tenant)?; - let db = catalog.get_database(&tenant, &self.plan.database).await?; - role_api - .drop_ownership(&GrantObjectByID::Table { - catalog_name: self.plan.catalog.clone(), - db_id: db.get_db_info().ident.db_id, - table_id: tbl.get_table_info().ident.table_id, - }) - .await?; + let catalog = self.ctx.get_catalog(catalog_name).await?; - // Although even if data is in READ_ONLY mode, - // as a catalog object, the table itself is allowed to be dropped (and undropped later), - // `drop table ALL` is NOT allowed, which implies that the table data need to be truncated. - if self.plan.all { - // check mutability, if the table is read only, we cannot truncate the data - tbl.check_mutable().map_err(|e| { + // Although even if data is in READ_ONLY mode, + // as a catalog object, the table itself is allowed to be dropped (and undropped later), + // `drop table ALL` is NOT allowed, which implies that the table data need to be truncated. + if self.plan.all { + // check mutability, if the table is read only, we cannot truncate the data + tbl.check_mutable().map_err(|e| { e.add_message(" drop table ALL is not allowed for read only table, please consider remove the option ALL") })? - } + } - // actually drop table - let resp = catalog - .drop_table_by_id(DropTableByIdReq { - if_exists: self.plan.if_exists, - tenant, - table_name: tbl_name.to_string(), - tb_id: tbl.get_table_info().ident.table_id, - db_id: db.get_db_info().ident.db_id, - }) - .await?; + let tenant = self.ctx.get_tenant(); + let db = catalog.get_database(&tenant, &self.plan.database).await?; + // actually drop table + let resp = catalog + .drop_table_by_id(DropTableByIdReq { + if_exists: self.plan.if_exists, + tenant, + table_name: tbl_name.to_string(), + tb_id: tbl.get_table_info().ident.table_id, + db_id: db.get_db_info().ident.db_id, + }) + .await?; - // if `plan.all`, truncate, then purge the historical data - if self.plan.all { - // the above `catalog.drop_table` operation changed the table meta version, - // thus if we do not refresh the table instance, `truncate` will fail - let latest = tbl.as_ref().refresh(self.ctx.as_ref()).await?; - let maybe_fuse_table = FuseTable::try_from_table(latest.as_ref()); - // if target table if of type FuseTable, purge its historical data - // otherwise, plain truncate - if let Ok(fuse_table) = maybe_fuse_table { - let purge = true; - fuse_table.do_truncate(self.ctx.clone(), purge).await? - } else { - latest.truncate(self.ctx.clone()).await? - } - } + // we should do `drop ownership` after actually drop table, otherwise when we drop the ownership, + // but the table still exists, in the interval maybe some unexpected things will happen. + // drop the ownership + let role_api = UserApiProvider::instance().get_role_api_client(&self.plan.tenant)?; + role_api + .drop_ownership(&GrantObjectByID::Table { + catalog_name: self.plan.catalog.clone(), + db_id: db.get_db_info().ident.db_id, + table_id: tbl.get_table_info().ident.table_id, + }) + .await?; - // update share spec if needed - if let Some((spec_vec, share_table_info)) = resp.spec_vec { - save_share_spec( - &self.ctx.get_tenant(), - self.ctx.get_data_operator()?.operator(), - Some(spec_vec), - Some(share_table_info), - ) - .await?; + // if `plan.all`, truncate, then purge the historical data + if self.plan.all { + // the above `catalog.drop_table` operation changed the table meta version, + // thus if we do not refresh the table instance, `truncate` will fail + let latest = tbl.as_ref().refresh(self.ctx.as_ref()).await?; + let maybe_fuse_table = FuseTable::try_from_table(latest.as_ref()); + // if target table if of type FuseTable, purge its historical data + // otherwise, plain truncate + if let Ok(fuse_table) = maybe_fuse_table { + let purge = true; + fuse_table.do_truncate(self.ctx.clone(), purge).await? + } else { + latest.truncate(self.ctx.clone()).await? } } + // update share spec if needed + if let Some((spec_vec, share_table_info)) = resp.spec_vec { + save_share_spec( + &self.ctx.get_tenant(), + self.ctx.get_data_operator()?.operator(), + Some(spec_vec), + Some(share_table_info), + ) + .await?; + } + Ok(PipelineBuildResult::create()) } } diff --git a/src/query/service/src/interpreters/interpreter_table_set_options.rs b/src/query/service/src/interpreters/interpreter_table_set_options.rs index 1ffef79d48c2..0406e0a9e6b2 100644 --- a/src/query/service/src/interpreters/interpreter_table_set_options.rs +++ b/src/query/service/src/interpreters/interpreter_table_set_options.rs @@ -89,24 +89,13 @@ impl Interpreter for SetOptionsInterpreter { } let catalog = self.ctx.get_catalog(self.plan.catalog.as_str()).await?; let database = self.plan.database.as_str(); - let table = self.plan.table.as_str(); - let tbl = catalog - .get_table(self.ctx.get_tenant().as_str(), database, table) - .await - .ok(); + let table_name = self.plan.table.as_str(); + let table = catalog + .get_table(self.ctx.get_tenant().as_str(), database, table_name) + .await?; - let table = if let Some(table) = &tbl { - // check mutability - table.check_mutable()?; - table - } else { - return Err(ErrorCode::UnknownTable(format!( - "Unknown table `{}`.`{}` in catalog '{}'", - database, - self.plan.table.as_str(), - &catalog.name() - ))); - }; + // check mutability + table.check_mutable()?; // check bloom_index_columns. is_valid_bloom_index_columns(&self.plan.set_options, table.schema())?; diff --git a/tests/sqllogictests/suites/base/05_ddl/05_0000_ddl_drop_tables.test b/tests/sqllogictests/suites/base/05_ddl/05_0000_ddl_drop_tables.test index ee5af64534cb..8abdec94f420 100644 --- a/tests/sqllogictests/suites/base/05_ddl/05_0000_ddl_drop_tables.test +++ b/tests/sqllogictests/suites/base/05_ddl/05_0000_ddl_drop_tables.test @@ -13,6 +13,18 @@ DROP TABLE IF EXISTS t statement error 1025 DROP TABLE t +statement ok +DROP TABLE if exists database_error.t + +statement ok +DROP TABLE IF EXISTS catalog_error.database_error.t + +statement error 1003 +DROP TABLE database_error.t + +statement error 1119 +DROP TABLE catalog_error.database_error.t + statement error 1025 DROP TABLE system.abc diff --git a/tests/sqllogictests/suites/base/05_ddl/05_0034_ddl_alter_table_set_options_error.test b/tests/sqllogictests/suites/base/05_ddl/05_0034_ddl_alter_table_set_options_error.test new file mode 100644 index 000000000000..b3174a76f596 --- /dev/null +++ b/tests/sqllogictests/suites/base/05_ddl/05_0034_ddl_alter_table_set_options_error.test @@ -0,0 +1,8 @@ +statement error 1025 +alter table t2 set options(block_per_segment = 100) + +statement error 1003 +alter table database_error.t2 set options(block_per_segment = 100) + +statement error 1119 +alter table catalog_error.default.t2 set options(block_per_segment = 100) \ No newline at end of file diff --git a/tests/suites/0_stateless/20+_others/20_0013_pretty_error.result b/tests/suites/0_stateless/20+_others/20_0013_pretty_error.result index 546173caba7e..db272f109ea5 100644 --- a/tests/suites/0_stateless/20+_others/20_0013_pretty_error.result +++ b/tests/suites/0_stateless/20+_others/20_0013_pretty_error.result @@ -45,4 +45,4 @@ Error: APIError: ResponseError with 1065: error: | ^ column c specified in USING clause does not exist in left table -Error: APIError: ResponseError with 1025: Unknown table `default`.`t` in catalog 'default' +Error: APIError: ResponseError with 1025: Unknown table 't'