refactor: refactor error message return for drop table and set options

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())
     }
 }

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())?;

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
 

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)

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'