From 71255b3cbdfa5580b85e4f38453a28a25c12ffa0 Mon Sep 17 00:00:00 2001 From: LFC <990479+MichaelScofield@users.noreply.github.com> Date: Thu, 10 Apr 2025 18:08:45 +0800 Subject: [PATCH] refactor: avoid empty display in errors (#5858) * refactor: avoid empty display in errors * fix: resolve PR comments --- Cargo.lock | 1 + src/common/error/Cargo.toml | 3 + src/common/error/src/ext.rs | 10 +- src/common/error/tests/ext.rs | 111 +++++++++++++++++++++ src/common/macro/src/stack_trace_debug.rs | 63 +++++++++++- src/common/recordbatch/src/adapter.rs | 2 +- src/common/recordbatch/src/error.rs | 2 +- src/query/src/datafusion.rs | 9 +- src/query/src/datafusion/planner.rs | 6 +- src/query/src/error.rs | 2 +- src/query/src/plan.rs | 5 +- src/query/src/planner.rs | 14 +-- src/query/src/range_select/plan.rs | 37 ++++--- src/query/src/range_select/plan_rewrite.rs | 38 ++----- src/query/src/sql.rs | 8 +- 15 files changed, 227 insertions(+), 84 deletions(-) create mode 100644 src/common/error/tests/ext.rs diff --git a/Cargo.lock b/Cargo.lock index cba1fa8793..da4306f527 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1988,6 +1988,7 @@ dependencies = [ name = "common-error" version = "0.14.0" dependencies = [ + "common-macro", "http 1.1.0", "snafu 0.8.5", "strum 0.27.1", diff --git a/src/common/error/Cargo.toml b/src/common/error/Cargo.toml index 148e2c6633..031f944dbd 100644 --- a/src/common/error/Cargo.toml +++ b/src/common/error/Cargo.toml @@ -12,3 +12,6 @@ http.workspace = true snafu.workspace = true strum.workspace = true tonic.workspace = true + +[dev-dependencies] +common-macro.workspace = true diff --git a/src/common/error/src/ext.rs b/src/common/error/src/ext.rs index 3b4d15a835..3f95c5fe1a 100644 --- a/src/common/error/src/ext.rs +++ b/src/common/error/src/ext.rs @@ -42,7 +42,7 @@ pub trait ErrorExt: StackError { if let Some(external_error) = error.source() { let external_root = external_error.sources().last().unwrap(); - if error.to_string().is_empty() { + if error.transparent() { format!("{external_root}") } else { format!("{error}: {external_root}") @@ -86,6 +86,14 @@ pub trait StackError: std::error::Error { } result } + + /// Indicates whether this error is "transparent", that it delegates its "display" and "source" + /// to the underlying error. Could be useful when you are just wrapping some external error, + /// **AND** can not or would not provide meaningful contextual info. For example, the + /// `DataFusionError`. + fn transparent(&self) -> bool { + false + } } impl StackError for Arc { diff --git a/src/common/error/tests/ext.rs b/src/common/error/tests/ext.rs new file mode 100644 index 0000000000..0a39ed51c6 --- /dev/null +++ b/src/common/error/tests/ext.rs @@ -0,0 +1,111 @@ +// 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::any::Any; + +use common_error::ext::{ErrorExt, PlainError, StackError}; +use common_error::status_code::StatusCode; +use common_macro::stack_trace_debug; +use snafu::{Location, ResultExt, Snafu}; + +#[derive(Snafu)] +#[stack_trace_debug] +enum MyError { + #[snafu(display(r#"A normal error with "display" attribute, message "{}""#, message))] + Normal { + message: String, + #[snafu(source)] + error: PlainError, + #[snafu(implicit)] + location: Location, + }, + + #[snafu(transparent)] + Transparent { + #[snafu(source)] + error: PlainError, + #[snafu(implicit)] + location: Location, + }, +} + +impl ErrorExt for MyError { + fn status_code(&self) -> StatusCode { + StatusCode::Unexpected + } + + fn as_any(&self) -> &dyn Any { + self + } +} + +fn normal_error() -> Result<(), MyError> { + let plain_error = PlainError::new("".to_string(), StatusCode::Unexpected); + Err(plain_error).context(NormalSnafu { message: "blabla" }) +} + +fn transparent_error() -> Result<(), MyError> { + let plain_error = PlainError::new("".to_string(), StatusCode::Unexpected); + Err(plain_error)? +} + +#[test] +fn test_output_msg() { + let result = normal_error(); + assert_eq!( + result.unwrap_err().output_msg(), + r#"A normal error with "display" attribute, message "blabla": "# + ); + + let result = transparent_error(); + assert_eq!(result.unwrap_err().output_msg(), ""); +} + +#[test] +fn test_to_string() { + let result = normal_error(); + assert_eq!( + result.unwrap_err().to_string(), + r#"A normal error with "display" attribute, message "blabla""# + ); + + let result = transparent_error(); + assert_eq!(result.unwrap_err().to_string(), ""); +} + +#[test] +fn test_debug_format() { + let result = normal_error(); + assert_eq!( + format!("{:?}", result.unwrap_err()), + r#"0: A normal error with "display" attribute, message "blabla", at src/common/error/tests/ext.rs:55:22 +1: PlainError { msg: "", status_code: Unexpected }"# + ); + + let result = transparent_error(); + assert_eq!( + format!("{:?}", result.unwrap_err()), + r#"0: , at src/common/error/tests/ext.rs:60:5 +1: PlainError { msg: "", status_code: Unexpected }"# + ); +} + +#[test] +fn test_transparent_flag() { + let result = normal_error(); + assert!(!result.unwrap_err().transparent()); + + let result = transparent_error(); + assert!(result.unwrap_err().transparent()); +} diff --git a/src/common/macro/src/stack_trace_debug.rs b/src/common/macro/src/stack_trace_debug.rs index fbc24260f1..f82f4746d3 100644 --- a/src/common/macro/src/stack_trace_debug.rs +++ b/src/common/macro/src/stack_trace_debug.rs @@ -14,7 +14,7 @@ //! implement `::common_error::ext::StackError` -use proc_macro2::{Span, TokenStream as TokenStream2}; +use proc_macro2::{Literal, Span, TokenStream as TokenStream2, TokenTree}; use quote::{quote, quote_spanned}; use syn::spanned::Spanned; use syn::{parenthesized, Attribute, Ident, ItemEnum, Variant}; @@ -32,6 +32,7 @@ pub fn stack_trace_style_impl(args: TokenStream2, input: TokenStream2) -> TokenS variants.push(variant); } + let transparent_fn = build_transparent_fn(enum_name.clone(), &variants); let debug_fmt_fn = build_debug_fmt_impl(enum_name.clone(), variants.clone()); let next_fn = build_next_impl(enum_name.clone(), variants); let debug_impl = build_debug_impl(enum_name.clone()); @@ -43,6 +44,7 @@ pub fn stack_trace_style_impl(args: TokenStream2, input: TokenStream2) -> TokenS impl ::common_error::ext::StackError for #enum_name { #debug_fmt_fn #next_fn + #transparent_fn } #debug_impl @@ -115,6 +117,7 @@ struct ErrorVariant { has_source: bool, has_external_cause: bool, display: TokenStream2, + transparent: bool, span: Span, cfg_attr: Option, } @@ -140,6 +143,7 @@ impl ErrorVariant { } let mut display = None; + let mut transparent = false; let mut cfg_attr = None; for attr in variant.attrs { if attr.path().is_ident("snafu") { @@ -150,17 +154,29 @@ impl ErrorVariant { let display_ts: TokenStream2 = content.parse()?; display = Some(display_ts); Ok(()) + } else if meta.path.is_ident("transparent") { + display = Some(TokenStream2::from(TokenTree::Literal(Literal::string( + "", + )))); + transparent = true; + Ok(()) } else { Err(meta.error("unrecognized repr")) } }) - .expect("Each error should contains a display attribute"); + .unwrap_or_else(|e| panic!("{e}")); } if attr.path().is_ident("cfg") { cfg_attr = Some(attr); } } + let display = display.unwrap_or_else(|| { + panic!( + r#"Error "{}" must be annotated with attribute "display" or "transparent"."#, + variant.ident, + ) + }); let field_ident = variant .fields @@ -174,7 +190,8 @@ impl ErrorVariant { has_location, has_source, has_external_cause, - display: display.unwrap(), + display, + transparent, span, cfg_attr, } @@ -275,4 +292,44 @@ impl ErrorVariant { } } } + + fn build_transparent_match_arm(&self) -> TokenStream2 { + let cfg = if let Some(cfg) = &self.cfg_attr { + quote_spanned!(cfg.span() => #cfg) + } else { + quote! {} + }; + let name = &self.name; + let fields = &self.fields; + + if self.transparent { + quote_spanned! { + self.span => #cfg #[allow(unused_variables)] #name { #(#fields),* } => { + true + }, + } + } else { + quote_spanned! { + self.span => #cfg #[allow(unused_variables)] #name { #(#fields),* } =>{ + false + } + } + } + } +} + +fn build_transparent_fn(enum_name: Ident, variants: &[ErrorVariant]) -> TokenStream2 { + let match_arms = variants + .iter() + .map(|v| v.build_transparent_match_arm()) + .collect::>(); + + quote! { + fn transparent(&self) -> bool { + use #enum_name::*; + match self { + #(#match_arms)* + } + } + } } diff --git a/src/common/recordbatch/src/adapter.rs b/src/common/recordbatch/src/adapter.rs index d342aa8129..3d27d7120f 100644 --- a/src/common/recordbatch/src/adapter.rs +++ b/src/common/recordbatch/src/adapter.rs @@ -298,7 +298,7 @@ impl Stream for RecordBatchStreamAdapter { match Pin::new(&mut self.stream).poll_next(cx) { Poll::Pending => Poll::Pending, Poll::Ready(Some(df_record_batch)) => { - let df_record_batch = df_record_batch.context(error::PollStreamSnafu)?; + let df_record_batch = df_record_batch?; Poll::Ready(Some(RecordBatch::try_from_df_record_batch( self.schema(), df_record_batch, diff --git a/src/common/recordbatch/src/error.rs b/src/common/recordbatch/src/error.rs index 6a1c61c0a0..dfd85e4aa1 100644 --- a/src/common/recordbatch/src/error.rs +++ b/src/common/recordbatch/src/error.rs @@ -65,7 +65,7 @@ pub enum Error { location: Location, }, - #[snafu(display(""))] + #[snafu(transparent)] PollStream { #[snafu(source)] error: datafusion::error::DataFusionError, diff --git a/src/query/src/datafusion.rs b/src/query/src/datafusion.rs index e0f020cd3a..dba7d0215a 100644 --- a/src/query/src/datafusion.rs +++ b/src/query/src/datafusion.rs @@ -50,9 +50,9 @@ use crate::dataframe::DataFrame; pub use crate::datafusion::planner::DfContextProviderAdapter; use crate::dist_plan::MergeScanLogicalPlan; use crate::error::{ - CatalogSnafu, ConvertSchemaSnafu, CreateRecordBatchSnafu, DataFusionSnafu, - MissingTableMutationHandlerSnafu, MissingTimestampColumnSnafu, QueryExecutionSnafu, Result, - TableMutationSnafu, TableNotFoundSnafu, TableReadOnlySnafu, UnsupportedExprSnafu, + CatalogSnafu, ConvertSchemaSnafu, CreateRecordBatchSnafu, MissingTableMutationHandlerSnafu, + MissingTimestampColumnSnafu, QueryExecutionSnafu, Result, TableMutationSnafu, + TableNotFoundSnafu, TableReadOnlySnafu, UnsupportedExprSnafu, }; use crate::executor::QueryExecutor; use crate::metrics::{OnDone, QUERY_STAGE_ELAPSED}; @@ -308,8 +308,7 @@ impl DatafusionQueryEngine { let physical_plan = state .query_planner() .create_physical_plan(&optimized_plan, state) - .await - .context(DataFusionSnafu)?; + .await?; Ok(physical_plan) } diff --git a/src/query/src/datafusion/planner.rs b/src/query/src/datafusion/planner.rs index 912393690d..0ad531541f 100644 --- a/src/query/src/datafusion/planner.rs +++ b/src/query/src/datafusion/planner.rs @@ -43,7 +43,7 @@ use datafusion_sql::parser::Statement as DfStatement; use session::context::QueryContextRef; use snafu::{Location, ResultExt}; -use crate::error::{CatalogSnafu, DataFusionSnafu, Result}; +use crate::error::{CatalogSnafu, Result}; use crate::query_engine::{DefaultPlanDecoder, QueryEngineState}; pub struct DfContextProviderAdapter { @@ -70,9 +70,7 @@ impl DfContextProviderAdapter { query_ctx: QueryContextRef, ) -> Result { let table_names = if let Some(df_stmt) = df_stmt { - session_state - .resolve_table_references(df_stmt) - .context(DataFusionSnafu)? + session_state.resolve_table_references(df_stmt)? } else { vec![] }; diff --git a/src/query/src/error.rs b/src/query/src/error.rs index 1ebba8de5d..c2a2e960b0 100644 --- a/src/query/src/error.rs +++ b/src/query/src/error.rs @@ -126,7 +126,7 @@ pub enum Error { location: Location, }, - #[snafu(display(""))] + #[snafu(transparent)] DataFusion { #[snafu(source)] error: DataFusionError, diff --git a/src/query/src/plan.rs b/src/query/src/plan.rs index e94c073c70..8d5586607e 100644 --- a/src/query/src/plan.rs +++ b/src/query/src/plan.rs @@ -19,12 +19,11 @@ use datafusion_common::tree_node::{Transformed, TreeNode, TreeNodeRewriter}; use datafusion_common::TableReference; use datafusion_expr::{BinaryExpr, Expr, Join, LogicalPlan, Operator}; use session::context::QueryContextRef; -use snafu::ResultExt; pub use table::metadata::TableType; use table::table::adapter::DfTableProviderAdapter; use table::table_name::TableName; -use crate::error::{DataFusionSnafu, Result}; +use crate::error::Result; struct TableNamesExtractAndRewriter { pub(crate) table_names: HashSet, @@ -119,7 +118,7 @@ pub fn extract_and_rewrite_full_table_names( query_ctx: QueryContextRef, ) -> Result<(HashSet, LogicalPlan)> { let mut extractor = TableNamesExtractAndRewriter::new(query_ctx); - let plan = plan.rewrite(&mut extractor).context(DataFusionSnafu)?; + let plan = plan.rewrite(&mut extractor)?; Ok((extractor.table_names, plan.data)) } diff --git a/src/query/src/planner.rs b/src/query/src/planner.rs index b0d0063d70..e3ee3904b4 100644 --- a/src/query/src/planner.rs +++ b/src/query/src/planner.rs @@ -31,7 +31,7 @@ use snafu::ResultExt; use sql::ast::Expr as SqlExpr; use sql::statements::statement::Statement; -use crate::error::{DataFusionSnafu, PlanSqlSnafu, QueryPlanSnafu, Result, SqlSnafu}; +use crate::error::{PlanSqlSnafu, QueryPlanSnafu, Result, SqlSnafu}; use crate::log_query::planner::LogQueryPlanner; use crate::parser::QueryStatement; use crate::promql::planner::PromPlanner; @@ -118,8 +118,7 @@ impl DfLogicalPlanner { let context = QueryEngineContext::new(self.session_state.clone(), query_ctx); let plan = self .engine_state - .optimize_by_extension_rules(plan, &context) - .context(DataFusionSnafu)?; + .optimize_by_extension_rules(plan, &context)?; common_telemetry::debug!("Logical planner, optimize result: {plan}"); Ok(plan) @@ -154,9 +153,7 @@ impl DfLogicalPlanner { let sql_to_rel = SqlToRel::new_with_options(&context_provider, parser_options); - sql_to_rel - .sql_to_expr(sql.into(), schema, &mut PlannerContext::new()) - .context(DataFusionSnafu) + Ok(sql_to_rel.sql_to_expr(sql.into(), schema, &mut PlannerContext::new())?) } #[tracing::instrument(skip_all)] @@ -183,10 +180,7 @@ impl DfLogicalPlanner { #[tracing::instrument(skip_all)] fn optimize_logical_plan(&self, plan: LogicalPlan) -> Result { - self.engine_state - .optimize_logical_plan(plan) - .context(DataFusionSnafu) - .map(Into::into) + Ok(self.engine_state.optimize_logical_plan(plan)?) } } diff --git a/src/query/src/range_select/plan.rs b/src/query/src/range_select/plan.rs index eb28aacf1e..f2a25997bc 100644 --- a/src/query/src/range_select/plan.rs +++ b/src/query/src/range_select/plan.rs @@ -55,9 +55,9 @@ use datatypes::arrow::record_batch::RecordBatch; use datatypes::arrow::row::{OwnedRow, RowConverter, SortField}; use futures::{ready, Stream}; use futures_util::StreamExt; -use snafu::{ensure, ResultExt}; +use snafu::ensure; -use crate::error::{DataFusionSnafu, RangeQuerySnafu, Result}; +use crate::error::{RangeQuerySnafu, Result}; type Millisecond = ::Native; @@ -373,25 +373,22 @@ impl RangeSelect { Ok((None, Arc::new(field))) }, ) - .collect::>>() - .context(DataFusionSnafu)?; + .collect::>>()?; // add align_ts - let ts_field = time_index - .to_field(input.schema().as_ref()) - .context(DataFusionSnafu)?; + let ts_field = time_index.to_field(input.schema().as_ref())?; let time_index_name = ts_field.1.name().clone(); fields.push(ts_field); // add by - let by_fields = exprlist_to_fields(&by, &input).context(DataFusionSnafu)?; + let by_fields = exprlist_to_fields(&by, &input)?; fields.extend(by_fields.clone()); - let schema_before_project = Arc::new( - DFSchema::new_with_metadata(fields, input.schema().metadata().clone()) - .context(DataFusionSnafu)?, - ); - let by_schema = Arc::new( - DFSchema::new_with_metadata(by_fields, input.schema().metadata().clone()) - .context(DataFusionSnafu)?, - ); + let schema_before_project = Arc::new(DFSchema::new_with_metadata( + fields, + input.schema().metadata().clone(), + )?); + let by_schema = Arc::new(DFSchema::new_with_metadata( + by_fields, + input.schema().metadata().clone(), + )?); // If the results of project plan can be obtained directly from range plan without any additional // calculations, no project plan is required. We can simply project the final output of the range // plan to produce the final result. @@ -421,10 +418,10 @@ impl RangeSelect { (f.0.cloned(), Arc::new(f.1.clone())) }) .collect(); - Arc::new( - DFSchema::new_with_metadata(project_field, input.schema().metadata().clone()) - .context(DataFusionSnafu)?, - ) + Arc::new(DFSchema::new_with_metadata( + project_field, + input.schema().metadata().clone(), + )?) } else { schema_before_project.clone() }; diff --git a/src/query/src/range_select/plan_rewrite.rs b/src/query/src/range_select/plan_rewrite.rs index ff05a26706..b53e1079b8 100644 --- a/src/query/src/range_select/plan_rewrite.rs +++ b/src/query/src/range_select/plan_rewrite.rs @@ -43,8 +43,7 @@ use snafu::{ensure, OptionExt, ResultExt}; use table::table::adapter::DfTableProviderAdapter; use crate::error::{ - CatalogSnafu, DataFusionSnafu, RangeQuerySnafu, Result, TimeIndexNotFoundSnafu, - UnknownTableSnafu, + CatalogSnafu, RangeQuerySnafu, Result, TimeIndexNotFoundSnafu, UnknownTableSnafu, }; use crate::plan::ExtractExpr; use crate::range_select::plan::{Fill, RangeFn, RangeSelect}; @@ -385,8 +384,7 @@ impl RangePlanRewriter { let new_expr = expr .iter() .map(|expr| expr.clone().rewrite(&mut range_rewriter).map(|x| x.data)) - .collect::>>() - .context(DataFusionSnafu)?; + .collect::>>()?; if range_rewriter.by.is_empty() { range_rewriter.by = default_by; } @@ -408,9 +406,7 @@ impl RangePlanRewriter { } else { let project_plan = LogicalPlanBuilder::from(range_plan) .project(new_expr) - .context(DataFusionSnafu)? - .build() - .context(DataFusionSnafu)?; + .and_then(|x| x.build())?; Ok(Some(project_plan)) } } @@ -436,8 +432,7 @@ impl RangePlanRewriter { } ); LogicalPlanBuilder::from(inputs[0].clone()) - .explain(*verbose, true) - .context(DataFusionSnafu)? + .explain(*verbose, true)? .build() } LogicalPlan::Explain(Explain { verbose, .. }) => { @@ -448,8 +443,7 @@ impl RangePlanRewriter { } ); LogicalPlanBuilder::from(inputs[0].clone()) - .explain(*verbose, false) - .context(DataFusionSnafu)? + .explain(*verbose, false)? .build() } LogicalPlan::Distinct(Distinct::On(DistinctOn { @@ -470,13 +464,11 @@ impl RangePlanRewriter { on_expr.clone(), select_expr.clone(), sort_expr.clone(), - ) - .context(DataFusionSnafu)? + )? .build() } _ => plan.with_new_exprs(plan.expressions_consider_join(), inputs), - } - .context(DataFusionSnafu)?; + }?; Ok(Some(plan)) } else { Ok(None) @@ -606,8 +598,6 @@ fn interval_only_in_expr(expr: &Expr) -> bool { #[cfg(test)] mod test { - use std::error::Error; - use arrow::datatypes::IntervalUnit; use catalog::memory::MemoryCatalogManager; use catalog::RegisterTableRequest; @@ -825,12 +815,7 @@ mod test { /// the right argument is `range_fn(avg(field_0), '5m', 'NULL', '0', '1h')` async fn range_argument_err_1() { let query = r#"SELECT range_fn('5m', avg(field_0), 'NULL', '1', tag_0, '1h') FROM test group by tag_0;"#; - let error = do_query(query) - .await - .unwrap_err() - .source() - .unwrap() - .to_string(); + let error = do_query(query).await.unwrap_err().to_string(); assert_eq!( error, "Error during planning: Illegal argument `Utf8(\"5m\")` in range select query" @@ -840,12 +825,7 @@ mod test { #[tokio::test] async fn range_argument_err_2() { let query = r#"SELECT range_fn(avg(field_0), 5, 'NULL', '1', tag_0, '1h') FROM test group by tag_0;"#; - let error = do_query(query) - .await - .unwrap_err() - .source() - .unwrap() - .to_string(); + let error = do_query(query).await.unwrap_err().to_string(); assert_eq!( error, "Error during planning: Illegal argument `Int64(5)` in range select query" diff --git a/src/query/src/sql.rs b/src/query/src/sql.rs index fbda344427..b62289fb6b 100644 --- a/src/query/src/sql.rs +++ b/src/query/src/sql.rs @@ -301,8 +301,7 @@ async fn query_from_information_schema_table( .state() .clone(), ) - .read_table(view) - .context(error::DataFusionSnafu)?; + .read_table(view)?; let planner = query_engine.planner(); let planner = planner @@ -319,10 +318,7 @@ async fn query_from_information_schema_table( } }; - let stream = dataframe - .execute_stream() - .await - .context(error::DataFusionSnafu)?; + let stream = dataframe.execute_stream().await?; Ok(Output::new_with_stream(Box::pin( RecordBatchStreamAdapter::try_new(stream).context(error::CreateRecordBatchSnafu)?,