chore: Address CR comments

src/common/error/src/ext.rs

@@ -20,7 +20,7 @@ macro_rules! define_opaque_error {
         /// An error behaves like `Box<dyn Error>`.
         ///
         /// Define this error as a new type instead of using `Box<dyn Error>` directly so we can implement
-        /// more method or trait for it.
+        /// more methods or traits for it.
         pub struct $Error {
             inner: Box<dyn $crate::ext::ErrorExt + Send + Sync>,
         }

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

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");
+    }
+}

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

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<InnerError> 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> {

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: "",

src/table/src/error.rs

@@ -47,21 +47,21 @@ impl From<InnerError> 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());
     }