Skip to content

Commit

Permalink
Simplified implementation of error reporting
Browse files Browse the repository at this point in the history
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
  • Loading branch information
rben01 committed Oct 23, 2023
1 parent beea9d3 commit f801640
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 58 deletions.
18 changes: 5 additions & 13 deletions crates/polars-lazy/src/tests/err_msg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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}'"
)))
};
Expand Down
2 changes: 1 addition & 1 deletion crates/polars-plan/src/logical_plan/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
},
}
Expand Down
70 changes: 26 additions & 44 deletions crates/polars-plan/src/logical_plan/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Mutex<ErrorStateEncounters>>);
pub struct ErrorState(pub(crate) Arc<Mutex<ErrorStateUnsync>>);

impl std::fmt::Debug for ErrorState {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
// Skip over the Arc<Mutex<_>> and just print the ErrorStateEncounters
f.debug_tuple("ErrorState")
.field(&self.0.lock().unwrap())
let this = self.0.lock().unwrap();
// Skip over the Arc<Mutex<ErrorStateUnsync>> 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<PolarsError> 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
}
}

Expand Down

0 comments on commit f801640

Please sign in to comment.