From 5f48b4996bf92d7ee8fe65096e502955cd034734 Mon Sep 17 00:00:00 2001 From: evenyag Date: Mon, 9 May 2022 11:52:01 +0800 Subject: [PATCH] chore: Address CR comments --- src/common/error/src/ext.rs | 2 +- src/common/error/src/mock.rs | 2 +- src/common/error/src/status_code.rs | 34 +++++++++++++++++++++++++++++ src/datanode/src/error.rs | 6 ++--- src/query/src/datafusion/error.rs | 24 ++++++++++---------- src/sql/src/error.rs | 6 ++--- src/table/src/error.rs | 8 +++---- 7 files changed, 59 insertions(+), 23 deletions(-) diff --git a/src/common/error/src/ext.rs b/src/common/error/src/ext.rs index 31c8bf917a..98bb9a083a 100644 --- a/src/common/error/src/ext.rs +++ b/src/common/error/src/ext.rs @@ -20,7 +20,7 @@ macro_rules! define_opaque_error { /// An error behaves like `Box`. /// /// Define this error as a new type instead of using `Box` directly so we can implement - /// more method or trait for it. + /// more methods or traits for it. pub struct $Error { inner: Box, } diff --git a/src/common/error/src/mock.rs b/src/common/error/src/mock.rs index e031779b4a..8568d87e31 100644 --- a/src/common/error/src/mock.rs +++ b/src/common/error/src/mock.rs @@ -33,7 +33,7 @@ impl MockError { impl fmt::Display for MockError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{:?}", self.code) + write!(f, "{}", self.code) } } diff --git a/src/common/error/src/status_code.rs b/src/common/error/src/status_code.rs index e81cc349fc..0f3286e69e 100644 --- a/src/common/error/src/status_code.rs +++ b/src/common/error/src/status_code.rs @@ -1,3 +1,5 @@ +use std::fmt; + /// Common status code for public API. #[derive(Debug, Clone, Copy, PartialEq)] pub enum StatusCode { @@ -6,6 +8,8 @@ pub enum StatusCode { Unknown, /// Unsupported operation. Unsupported, + /// Unexpected error, maybe there is a BUG. + Unexpected, /// Internal server error. Internal, // ====== End of common status code ================ @@ -15,8 +19,38 @@ pub enum StatusCode { InvalidSyntax, // ====== End of SQL related status code =========== + // ====== Begin of query related status code ======= + /// Fail to create a plan for the query. + PlanQuery, + /// The query engine fail to execute query. + EngineExecuteQuery, + // ====== End of query related status code ========= + // ====== Begin of catalog related status code ===== /// Table already exists. TableAlreadyExists, // ====== End of catalog related status code ======= } + +impl fmt::Display for StatusCode { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + // The current debug format is suitable to display. + write!(f, "{:?}", self) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn assert_status_code_display(code: StatusCode, msg: &str) { + let code_msg = format!("{}", code); + assert_eq!(msg, code_msg); + } + + #[test] + fn test_display_status_code() { + assert_status_code_display(StatusCode::Unknown, "Unknown"); + assert_status_code_display(StatusCode::TableAlreadyExists, "TableAlreadyExists"); + } +} diff --git a/src/datanode/src/error.rs b/src/datanode/src/error.rs index 02c82b0061..195f4f0a94 100644 --- a/src/datanode/src/error.rs +++ b/src/datanode/src/error.rs @@ -44,7 +44,7 @@ mod tests { use super::*; - fn raise_query_error() -> std::result::Result<(), query::error::Error> { + fn throw_query_error() -> std::result::Result<(), query::error::Error> { Err(query::error::Error::new(MockError::with_backtrace( StatusCode::Internal, ))) @@ -57,9 +57,9 @@ mod tests { #[test] fn test_error() { - let err = raise_query_error().context(ExecuteSqlSnafu).err().unwrap(); + let err = throw_query_error().context(ExecuteSqlSnafu).err().unwrap(); assert_internal_error(&err); - let err = raise_query_error().context(NewCatalogSnafu).err().unwrap(); + let err = throw_query_error().context(NewCatalogSnafu).err().unwrap(); assert_internal_error(&err); } } diff --git a/src/query/src/datafusion/error.rs b/src/query/src/datafusion/error.rs index 425519de02..69c6a01501 100644 --- a/src/query/src/datafusion/error.rs +++ b/src/query/src/datafusion/error.rs @@ -37,10 +37,12 @@ impl ErrorExt for InnerError { use InnerError::*; match self { + // TODO(yingwen): Further categorize datafusion error. + Datafusion { .. } => StatusCode::EngineExecuteQuery, + // This downcast should not fail in usual case. + PhysicalPlanDowncast { .. } => StatusCode::Unexpected, ParseSql { source, .. } => source.status_code(), - Datafusion { .. } | PhysicalPlanDowncast { .. } | PlanSql { .. } => { - StatusCode::Internal - } + PlanSql { .. } => StatusCode::PlanQuery, } } @@ -59,32 +61,32 @@ impl From for Error { mod tests { use super::*; - fn raise_df_error() -> Result<(), DataFusionError> { + fn throw_df_error() -> Result<(), DataFusionError> { Err(DataFusionError::NotImplemented("test".to_string())) } - fn assert_internal_error(err: &InnerError) { - assert_eq!(StatusCode::Internal, err.status_code()); + fn assert_error(err: &InnerError, code: StatusCode) { + assert_eq!(code, err.status_code()); assert!(err.backtrace_opt().is_some()); } #[test] fn test_datafusion_as_source() { - let err = raise_df_error() + let err = throw_df_error() .context(DatafusionSnafu { msg: "test df" }) .err() .unwrap(); - assert_internal_error(&err); + assert_error(&err, StatusCode::EngineExecuteQuery); - let err = raise_df_error() + let err = throw_df_error() .context(PlanSqlSnafu { sql: "" }) .err() .unwrap(); - assert_internal_error(&err); + assert_error(&err, StatusCode::PlanQuery); let res: Result<(), InnerError> = PhysicalPlanDowncastSnafu {}.fail(); let err = res.err().unwrap(); - assert_internal_error(&err); + assert_error(&err, StatusCode::Unexpected); } fn raise_sql_error() -> Result<(), sql::error::Error> { diff --git a/src/sql/src/error.rs b/src/sql/src/error.rs index 1688a396f2..df0219c8d6 100644 --- a/src/sql/src/error.rs +++ b/src/sql/src/error.rs @@ -50,13 +50,13 @@ mod tests { use super::*; - fn raise_sp_error() -> Result<(), ParserError> { + fn throw_sp_error() -> Result<(), ParserError> { Err(ParserError::ParserError("parser error".to_string())) } #[test] fn test_syntax_error() { - let err = raise_sp_error() + let err = throw_sp_error() .context(SyntaxSnafu { sql: "" }) .err() .unwrap(); @@ -69,7 +69,7 @@ mod tests { ); assert_eq!(StatusCode::InvalidSyntax, err.status_code()); - let err = raise_sp_error() + let err = throw_sp_error() .context(UnexpectedSnafu { sql: "", expected: "", diff --git a/src/table/src/error.rs b/src/table/src/error.rs index e924a3d0ee..9e48a63248 100644 --- a/src/table/src/error.rs +++ b/src/table/src/error.rs @@ -47,21 +47,21 @@ impl From for DataFusionError { mod tests { use super::*; - fn raise_df_error() -> Result<()> { + fn throw_df_error() -> Result<()> { Err(DataFusionError::NotImplemented("table test".to_string())).context(DatafusionSnafu)? } - fn raise_repeatedly() -> Result<()> { + fn throw_repeatedly() -> Result<()> { ExecuteRepeatedlySnafu {}.fail()? } #[test] fn test_error() { - let err = raise_df_error().err().unwrap(); + let err = throw_df_error().err().unwrap(); assert!(err.backtrace_opt().is_some()); assert_eq!(StatusCode::Unknown, err.status_code()); - let err = raise_repeatedly().err().unwrap(); + let err = throw_repeatedly().err().unwrap(); assert!(err.backtrace_opt().is_some()); assert_eq!(StatusCode::Unknown, err.status_code()); }