From 3dac7cbe3750b69cde4c81d6b59062c48cd9355d Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Sun, 28 Apr 2024 19:18:55 +0800 Subject: [PATCH] refactor: remove location_opt and DebugFormat (#3830) Signed-off-by: Ruihang Xia --- src/common/base/src/buffer.rs | 8 -- src/common/datasource/src/error.rs | 30 ------ src/common/decimal/src/error.rs | 8 -- src/common/error/src/ext.rs | 21 +--- src/common/error/src/format.rs | 154 ----------------------------- src/common/error/src/lib.rs | 1 - src/common/error/src/mock.rs | 6 -- src/common/runtime/src/error.rs | 8 -- src/common/time/src/error.rs | 15 --- 9 files changed, 3 insertions(+), 248 deletions(-) delete mode 100644 src/common/error/src/format.rs diff --git a/src/common/base/src/buffer.rs b/src/common/base/src/buffer.rs index b472bbf5380e..1eb696e6671a 100644 --- a/src/common/base/src/buffer.rs +++ b/src/common/base/src/buffer.rs @@ -53,14 +53,6 @@ impl ErrorExt for Error { fn as_any(&self) -> &dyn Any { self } - - fn location_opt(&self) -> Option { - match self { - Error::Overflow { location, .. } => Some(*location), - Error::Underflow { location, .. } => Some(*location), - Error::Eof { location, .. } => Some(*location), - } - } } macro_rules! impl_read_le { diff --git a/src/common/datasource/src/error.rs b/src/common/datasource/src/error.rs index 77618345a3eb..fc2b81ecfc53 100644 --- a/src/common/datasource/src/error.rs +++ b/src/common/datasource/src/error.rs @@ -213,34 +213,4 @@ impl ErrorExt for Error { fn as_any(&self) -> &dyn Any { self } - - fn location_opt(&self) -> Option { - use Error::*; - match self { - OrcReader { location, .. } => Some(*location), - BuildBackend { location, .. } => Some(*location), - ReadObject { location, .. } => Some(*location), - ListObjects { location, .. } => Some(*location), - InferSchema { location, .. } => Some(*location), - ReadParquetSnafu { location, .. } => Some(*location), - ParquetToSchema { location, .. } => Some(*location), - JoinHandle { location, .. } => Some(*location), - ParseFormat { location, .. } => Some(*location), - MergeSchema { location, .. } => Some(*location), - WriteObject { location, .. } => Some(*location), - ReadRecordBatch { location, .. } => Some(*location), - WriteRecordBatch { location, .. } => Some(*location), - AsyncWrite { location, .. } => Some(*location), - EncodeRecordBatch { location, .. } => Some(*location), - BufferedWriterClosed { location, .. } => Some(*location), - - UnsupportedBackendProtocol { location, .. } => Some(*location), - EmptyHostPath { location, .. } => Some(*location), - InvalidUrl { location, .. } => Some(*location), - InvalidConnection { location, .. } => Some(*location), - UnsupportedCompressionType { location, .. } => Some(*location), - UnsupportedFormat { location, .. } => Some(*location), - WriteParquet { location, .. } => Some(*location), - } - } } diff --git a/src/common/decimal/src/error.rs b/src/common/decimal/src/error.rs index 8bfa3e9fe8f6..e65beb6e99e6 100644 --- a/src/common/decimal/src/error.rs +++ b/src/common/decimal/src/error.rs @@ -56,14 +56,6 @@ impl ErrorExt for Error { } } - fn location_opt(&self) -> Option { - match self { - Error::BigDecimalOutOfRange { location, .. } => Some(*location), - Error::InvalidPrecisionOrScale { location, .. } => Some(*location), - Error::ParseRustDecimalStr { .. } | Error::ParseBigDecimalStr { .. } => None, - } - } - fn as_any(&self) -> &dyn std::any::Any { self } diff --git a/src/common/error/src/ext.rs b/src/common/error/src/ext.rs index 690ea23dc3e2..166a0922e5e1 100644 --- a/src/common/error/src/ext.rs +++ b/src/common/error/src/ext.rs @@ -24,13 +24,6 @@ pub trait ErrorExt: StackError { StatusCode::Unknown } - // TODO(ruihang): remove this default implementation - /// Get the location of this error, None if the location is unavailable. - /// Add `_opt` suffix to avoid confusing with similar method in `std::error::Error` - fn location_opt(&self) -> Option { - None - } - /// Returns the error as [Any](std::any::Any) so that it can be /// downcast to a specific implementation. fn as_any(&self) -> &dyn Any; @@ -116,9 +109,9 @@ impl BoxedError { impl std::fmt::Debug for BoxedError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - // Use the pretty debug format of inner error for opaque error. - let debug_format = crate::format::DebugFormat::new(&*self.inner); - debug_format.fmt(f) + let mut buf = vec![]; + self.debug_fmt(0, &mut buf); + write!(f, "{}", buf.join("\n")) } } @@ -139,10 +132,6 @@ impl crate::ext::ErrorExt for BoxedError { self.inner.status_code() } - fn location_opt(&self) -> Option { - self.inner.location_opt() - } - fn as_any(&self) -> &dyn std::any::Any { self.inner.as_any() } @@ -196,10 +185,6 @@ impl crate::ext::ErrorExt for PlainError { self.status_code } - fn location_opt(&self) -> Option { - None - } - fn as_any(&self) -> &dyn std::any::Any { self as _ } diff --git a/src/common/error/src/format.rs b/src/common/error/src/format.rs deleted file mode 100644 index 341e4829e954..000000000000 --- a/src/common/error/src/format.rs +++ /dev/null @@ -1,154 +0,0 @@ -// Copyright 2023 Greptime Team -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -use std::fmt; - -use crate::ext::ErrorExt; - -/// Pretty debug format for error, also prints source and backtrace. -pub struct DebugFormat<'a, E: ?Sized>(&'a E); - -impl<'a, E: ?Sized> DebugFormat<'a, E> { - /// Create a new format struct from `err`. - pub fn new(err: &'a E) -> Self { - Self(err) - } -} - -impl<'a, E: ErrorExt + ?Sized> fmt::Debug for DebugFormat<'a, E> { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}.", self.0)?; - if let Some(source) = self.0.source() { - // Source error use debug format for more verbose info. - write!(f, " Caused by: {source:?}")?; - } - if let Some(location) = self.0.location_opt() { - // Add a newline to separate causes and backtrace. - write!(f, " at: {location}")?; - } - - Ok(()) - } -} - -#[cfg(test)] -mod tests { - use std::any::Any; - - use snafu::prelude::*; - use snafu::{GenerateImplicitData, Location}; - - use super::*; - use crate::ext::StackError; - - #[derive(Debug, Snafu)] - #[snafu(display("This is a leaf error"))] - struct Leaf; - - impl ErrorExt for Leaf { - fn location_opt(&self) -> Option { - None - } - - fn as_any(&self) -> &dyn Any { - self - } - } - - impl StackError for Leaf { - fn debug_fmt(&self, _: usize, _: &mut Vec) {} - - fn next(&self) -> Option<&dyn StackError> { - None - } - } - - #[derive(Debug, Snafu)] - #[snafu(display("This is a leaf with location"))] - struct LeafWithLocation { - location: Location, - } - - impl ErrorExt for LeafWithLocation { - fn location_opt(&self) -> Option { - None - } - - fn as_any(&self) -> &dyn Any { - self - } - } - - impl StackError for LeafWithLocation { - fn debug_fmt(&self, _: usize, _: &mut Vec) {} - - fn next(&self) -> Option<&dyn StackError> { - None - } - } - - #[derive(Debug, Snafu)] - #[snafu(display("Internal error"))] - struct Internal { - #[snafu(source)] - source: Leaf, - location: Location, - } - - impl ErrorExt for Internal { - fn location_opt(&self) -> Option { - None - } - - fn as_any(&self) -> &dyn Any { - self - } - } - - impl StackError for Internal { - fn debug_fmt(&self, layer: usize, buf: &mut Vec) { - buf.push(format!("{}: Internal error, at {}", layer, self.location)); - self.source.debug_fmt(layer + 1, buf); - } - - fn next(&self) -> Option<&dyn StackError> { - Some(&self.source) - } - } - - #[test] - fn test_debug_format() { - let err = Leaf; - - let msg = format!("{:?}", DebugFormat::new(&err)); - assert_eq!("This is a leaf error.", msg); - - let err = LeafWithLocation { - location: Location::generate(), - }; - - // TODO(ruihang): display location here - let msg = format!("{:?}", DebugFormat::new(&err)); - assert!(msg.starts_with("This is a leaf with location.")); - - let err = Internal { - source: Leaf, - location: Location::generate(), - }; - - // TODO(ruihang): display location here - let msg = format!("{:?}", DebugFormat::new(&err)); - assert!(msg.contains("Internal error. Caused by: Leaf")); - } -} diff --git a/src/common/error/src/lib.rs b/src/common/error/src/lib.rs index aa54ef39e78f..c5c0e6efe092 100644 --- a/src/common/error/src/lib.rs +++ b/src/common/error/src/lib.rs @@ -15,7 +15,6 @@ #![feature(error_iter)] pub mod ext; -pub mod format; pub mod mock; pub mod status_code; diff --git a/src/common/error/src/mock.rs b/src/common/error/src/mock.rs index 2781149b1268..572e11deabbf 100644 --- a/src/common/error/src/mock.rs +++ b/src/common/error/src/mock.rs @@ -17,8 +17,6 @@ use std::any::Any; use std::fmt; -use snafu::Location; - use crate::ext::{ErrorExt, StackError}; use crate::status_code::StatusCode; @@ -61,10 +59,6 @@ impl ErrorExt for MockError { self.code } - fn location_opt(&self) -> Option { - None - } - fn as_any(&self) -> &dyn Any { self } diff --git a/src/common/runtime/src/error.rs b/src/common/runtime/src/error.rs index 35d053510126..828306f76bc8 100644 --- a/src/common/runtime/src/error.rs +++ b/src/common/runtime/src/error.rs @@ -48,12 +48,4 @@ impl ErrorExt for Error { fn as_any(&self) -> &dyn Any { self } - - fn location_opt(&self) -> Option { - match self { - Error::BuildRuntime { location, .. } - | Error::IllegalState { location, .. } - | Error::WaitGcTaskStop { location, .. } => Some(*location), - } - } } diff --git a/src/common/time/src/error.rs b/src/common/time/src/error.rs index c4c025dc92cf..df4f0883b7a8 100644 --- a/src/common/time/src/error.rs +++ b/src/common/time/src/error.rs @@ -98,21 +98,6 @@ impl ErrorExt for Error { fn as_any(&self) -> &dyn Any { self } - - fn location_opt(&self) -> Option { - match self { - Error::ParseTimestamp { location, .. } - | Error::Format { location, .. } - | Error::TimestampOverflow { location, .. } - | Error::ArithmeticOverflow { location, .. } => Some(*location), - Error::ParseDateStr { .. } - | Error::InvalidTimezoneOffset { .. } - | Error::ParseOffsetStr { .. } - | Error::ParseTimezoneName { .. } => None, - Error::InvalidDateStr { location, .. } => Some(*location), - Error::ParseInterval { location, .. } => Some(*location), - } - } } pub type Result = std::result::Result;