From 7e2e3e342979941c16d3e6728c2e0a3b0e91c57b Mon Sep 17 00:00:00 2001 From: evenyag Date: Fri, 29 Apr 2022 16:51:55 +0800 Subject: [PATCH] feat: Impl ErrorExt for opaque error and ParseError --- Cargo.lock | 1 + src/common/error/src/ext.rs | 38 ++++++++++++++----- src/common/error/src/lib.rs | 10 +++++ src/common/error/src/status_code.rs | 11 +++++- src/query/src/error.rs | 1 + .../src/query_engine/datafusion/error.rs | 1 + src/sql/Cargo.toml | 1 + src/sql/src/errors.rs | 16 +++++++- 8 files changed, 68 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ab314d117d..fe9a95184e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1774,6 +1774,7 @@ dependencies = [ name = "sql" version = "0.1.0" dependencies = [ + "common-error", "snafu", "sqlparser", ] diff --git a/src/common/error/src/ext.rs b/src/common/error/src/ext.rs index be982096a4..deaf728601 100644 --- a/src/common/error/src/ext.rs +++ b/src/common/error/src/ext.rs @@ -2,14 +2,15 @@ use crate::status_code::StatusCode; /// Extension to [`Error`](std::error::Error) in std. pub trait ErrorExt: std::error::Error { - /// Returns the [StatusCode] of this holder. + /// Map this error to [StatusCode]. fn status_code(&self) -> StatusCode { StatusCode::Unknown } - /// Get the reference to the backtrace of this error. - // Add `_opt` suffix to avoid confusing with similar method in `std::error::Error`. - fn backtrace_opt(&self) -> Option<&snafu::Backtrace>; + /// Get the reference to the backtrace of this error, None if the backtrace is unavailable. + // Add `_opt` suffix to avoid confusing with similar method in `std::error::Error`, once backtrace + // in std is stable, we can deprecate this method. + fn backtrace_opt(&self) -> Option<&crate::snafu::Backtrace>; } /// A helper macro to define a opaque boxed error based on errors that implement [ErrorExt] trait. @@ -25,7 +26,6 @@ macro_rules! define_opaque_error { } impl $Error { - /// Create a new error. pub fn new(err: E) -> Self { Self { inner: Box::new(err), @@ -57,7 +57,13 @@ macro_rules! define_opaque_error { self.inner.status_code() } - fn backtrace_opt(&self) -> Option<&snafu::Backtrace> { + fn backtrace_opt(&self) -> Option<&$crate::snafu::Backtrace> { + self.inner.backtrace_opt() + } + } + + impl $crate::snafu::ErrorCompat for $Error { + fn backtrace(&self) -> Option<&$crate::snafu::Backtrace> { self.inner.backtrace_opt() } } @@ -68,7 +74,7 @@ macro_rules! define_opaque_error { mod tests { use std::error::Error as StdError; - use snafu::{prelude::*, Backtrace}; + use snafu::{prelude::*, Backtrace, ErrorCompat}; use super::*; @@ -88,7 +94,7 @@ mod tests { impl ErrorExt for InnerError { fn backtrace_opt(&self) -> Option<&snafu::Backtrace> { - snafu::ErrorCompat::backtrace(self) + ErrorCompat::backtrace(self) } } @@ -111,7 +117,7 @@ mod tests { } #[test] - fn test_opaque_error() { + fn test_inner_error() { let leaf = throw_leaf().err().unwrap(); assert!(leaf.backtrace_opt().is_some()); assert!(leaf.source().is_none()); @@ -120,4 +126,18 @@ mod tests { assert!(internal.backtrace_opt().is_some()); assert!(internal.source().is_some()); } + + #[test] + fn test_opaque_error() { + let err: Error = throw_leaf().map_err(Into::into).err().unwrap(); + let msg = format!("{:?}", err); + assert!(msg.contains("\nBacktrace:\n")); + assert!(ErrorCompat::backtrace(&err).is_some()); + + let err: Error = throw_internal().map_err(Into::into).err().unwrap(); + let msg = format!("{:?}", err); + assert!(msg.contains("\nBacktrace:\n")); + assert!(msg.contains("Caused by")); + assert!(ErrorCompat::backtrace(&err).is_some()); + } } diff --git a/src/common/error/src/lib.rs b/src/common/error/src/lib.rs index 98f017a44f..9ac962b859 100644 --- a/src/common/error/src/lib.rs +++ b/src/common/error/src/lib.rs @@ -1,3 +1,13 @@ pub mod ext; pub mod format; pub mod status_code; + +pub mod prelude { + pub use snafu::Backtrace; + + pub use crate::ext::ErrorExt; + pub use crate::format::DebugFormat; + pub use crate::status_code::StatusCode; +} + +pub use snafu; diff --git a/src/common/error/src/status_code.rs b/src/common/error/src/status_code.rs index 096d88ae4b..92f0ec2620 100644 --- a/src/common/error/src/status_code.rs +++ b/src/common/error/src/status_code.rs @@ -1,6 +1,15 @@ /// Common status code for public API. #[derive(Debug, Clone, Copy)] pub enum StatusCode { - /// Unknown status. + // ====== Begin of common status code =========== + /// Unknown error. Unknown, + /// Unsupported operation. + Unsupported, + // ====== End of common status code ============= + + // ====== Begin of SQL related status code ====== + /// SQL Syntax error. + InvalidSyntax, + // ====== End of SQL related status code ======== } diff --git a/src/query/src/error.rs b/src/query/src/error.rs index 1253da97d2..76cedc1872 100644 --- a/src/query/src/error.rs +++ b/src/query/src/error.rs @@ -1,4 +1,5 @@ use datafusion::error::DataFusionError; +use snafu::Snafu; common_error::define_opaque_error!(Error); diff --git a/src/query/src/query_engine/datafusion/error.rs b/src/query/src/query_engine/datafusion/error.rs index 50d6e96e0d..4cb207fcd1 100644 --- a/src/query/src/query_engine/datafusion/error.rs +++ b/src/query/src/query_engine/datafusion/error.rs @@ -33,6 +33,7 @@ pub enum InnerError { Execution { message: String }, } +// TODO(yingwen): Implement status_code(). impl ErrorExt for InnerError { fn backtrace_opt(&self) -> Option<&snafu::Backtrace> { ErrorCompat::backtrace(self) diff --git a/src/sql/Cargo.toml b/src/sql/Cargo.toml index a3c34f5e47..98365426a0 100644 --- a/src/sql/Cargo.toml +++ b/src/sql/Cargo.toml @@ -6,5 +6,6 @@ edition = "2021" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] +common-error = { path = "../common/error" } snafu = { version = "0.7", features = ["backtraces"] } sqlparser = "0.15.0" diff --git a/src/sql/src/errors.rs b/src/sql/src/errors.rs index d483dc2877..1bf4ed48a3 100644 --- a/src/sql/src/errors.rs +++ b/src/sql/src/errors.rs @@ -1,4 +1,5 @@ -use snafu::prelude::*; +use common_error::prelude::*; +use snafu::{prelude::*, ErrorCompat}; use sqlparser::parser::ParserError as SpParserError; /// SQL parser errors. @@ -29,6 +30,19 @@ pub enum ParserError { SpSyntax { sql: String, source: SpParserError }, } +impl ErrorExt for ParserError { + fn status_code(&self) -> StatusCode { + match self { + Self::Unsupported { .. } => StatusCode::Unsupported, + Self::Unexpected { .. } | Self::SpSyntax { .. } => StatusCode::InvalidSyntax, + } + } + + fn backtrace_opt(&self) -> Option<&Backtrace> { + ErrorCompat::backtrace(self) + } +} + #[cfg(test)] mod tests { use std::assert_matches::assert_matches;