From f801640545fde747159c4c25fe28f48165e711f3 Mon Sep 17 00:00:00 2001 From: Robert Bennett Date: Mon, 23 Oct 2023 00:55:09 -0400 Subject: [PATCH] Simplified implementation of error reporting Now that errors aren't taken and replaced by a String - the original is stored and cloned as needed - we can simplify the implementation. Instead of NotYetEncountered and AlreadyEncountered, we can just have a single struct with a counteer (no enum needed). This massively simplifies the implementation Also, simplified the implemtnation of tests Also cleaned up ErrorState's Debug implementation so that users don't see the nuts and bolts inside the struct - they only see the important debugging info --- crates/polars-lazy/src/tests/err_msg.rs | 18 ++--- .../polars-plan/src/logical_plan/builder.rs | 2 +- crates/polars-plan/src/logical_plan/mod.rs | 70 +++++++------------ 3 files changed, 32 insertions(+), 58 deletions(-) diff --git a/crates/polars-lazy/src/tests/err_msg.rs b/crates/polars-lazy/src/tests/err_msg.rs index c976a4d7a4623..6f1a40ba71dbe 100644 --- a/crates/polars-lazy/src/tests/err_msg.rs +++ b/crates/polars-lazy/src/tests/err_msg.rs @@ -29,21 +29,13 @@ fn col_not_found_error_messages() { operation:\n{INITIAL_PROJECTION_STR}" ); - let plan_err_str; - let collect_err; + let plan_err_str = + format!("ErrorState {{ n_times: {n}, err: ColumnNotFound(ErrString({err_msg:?})) }}"); - if n == 0 { - plan_err_str = format!( - "ErrorState(NotYetEncountered {{ \ - err: ColumnNotFound(ErrString({err_msg:?})) }})" - ); - collect_err = PolarsError::ColumnNotFound(ErrString::from(err_msg.to_owned())); + let collect_err = if n == 0 { + PolarsError::ColumnNotFound(ErrString::from(err_msg.to_owned())) } else { - plan_err_str = format!( - "ErrorState(AlreadyEncountered {{ n_times: {n}, \ - orig_err: ColumnNotFound(ErrString({err_msg:?})) }})", - ); - collect_err = PolarsError::ColumnNotFound(ErrString::from(format!( + PolarsError::ColumnNotFound(ErrString::from(format!( "LogicalPlan already failed (depth: {n}) with error: '{err_msg}'" ))) }; diff --git a/crates/polars-plan/src/logical_plan/builder.rs b/crates/polars-plan/src/logical_plan/builder.rs index 318a55a4a2101..0efb0c9ed959d 100644 --- a/crates/polars-plan/src/logical_plan/builder.rs +++ b/crates/polars-plan/src/logical_plan/builder.rs @@ -72,7 +72,7 @@ macro_rules! raise_err { LogicalPlan::Error { input: Box::new(input), - err: err.into(), // PolarsError -> ErrorState(NotYetEncountered) + err: err.into(), // PolarsError -> ErrorState } }, } diff --git a/crates/polars-plan/src/logical_plan/mod.rs b/crates/polars-plan/src/logical_plan/mod.rs index b8b4c05495e4c..ac79475b0ed80 100644 --- a/crates/polars-plan/src/logical_plan/mod.rs +++ b/crates/polars-plan/src/logical_plan/mod.rs @@ -69,69 +69,51 @@ pub enum Context { } #[derive(Debug)] -pub(crate) enum ErrorStateEncounters { - NotYetEncountered { - err: PolarsError, - }, - AlreadyEncountered { - n_times: usize, - orig_err: PolarsError, - }, +pub(crate) struct ErrorStateUnsync { + n_times: usize, + err: PolarsError, } #[derive(Clone)] -pub struct ErrorState(pub(crate) Arc>); +pub struct ErrorState(pub(crate) Arc>); impl std::fmt::Debug for ErrorState { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - // Skip over the Arc> and just print the ErrorStateEncounters - f.debug_tuple("ErrorState") - .field(&self.0.lock().unwrap()) + let this = self.0.lock().unwrap(); + // Skip over the Arc> and just print the fields we care + // about. Technically this is misleading, but the insides of ErrorState are not + // public, so this only affects authors of polars, not users (and the odds that + // this affects authors is slim) + f.debug_struct("ErrorState") + .field("n_times", &this.n_times) + .field("err", &this.err) .finish() } } impl From for ErrorState { fn from(err: PolarsError) -> Self { - Self(Arc::new(Mutex::new( - ErrorStateEncounters::NotYetEncountered { err }, - ))) + Self(Arc::new(Mutex::new(ErrorStateUnsync { n_times: 0, err }))) } } impl ErrorState { fn take(&self) -> PolarsError { - let mut curr_err = self.0.lock().unwrap(); + let mut this = self.0.lock().unwrap(); - match &mut *curr_err { - ErrorStateEncounters::NotYetEncountered { err: polars_err } => { - let orig_err = polars_err.wrap_msg(&str::to_owned); + let ret_err = if this.n_times == 0 { + this.err.wrap_msg(&|msg| msg.to_owned()) + } else { + this.err.wrap_msg(&|msg| { + format!( + "LogicalPlan already failed (depth: {}) with error: '{}'", + this.n_times, msg + ) + }) + }; + this.n_times += 1; - let prev_err = std::mem::replace( - &mut *curr_err, - ErrorStateEncounters::AlreadyEncountered { - n_times: 1, - orig_err, - }, - ); - // Since we're in this branch, we know err was a NotYetEncountered - match prev_err { - ErrorStateEncounters::NotYetEncountered { err } => err, - ErrorStateEncounters::AlreadyEncountered { .. } => unreachable!(), - } - }, - ErrorStateEncounters::AlreadyEncountered { n_times, orig_err } => { - let err = orig_err.wrap_msg(&|msg| { - format!( - "LogicalPlan already failed (depth: {}) with error: '{}'", - n_times, msg - ) - }); - - *n_times += 1; - err - }, - } + ret_err } }