-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make backtrace as a cargo feature #7527
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,8 +16,9 @@ | |
// under the License. | ||
|
||
//! DataFusion error types | ||
|
||
#[cfg(feature = "backtrace")] | ||
use std::backtrace::{Backtrace, BacktraceStatus}; | ||
|
||
use std::error::Error; | ||
use std::fmt::{Display, Formatter}; | ||
use std::io; | ||
|
@@ -368,6 +369,8 @@ impl From<DataFusionError> for io::Error { | |
} | ||
|
||
impl DataFusionError { | ||
const BACK_TRACE_SEP: &str = "\n\nbacktrace: "; | ||
|
||
/// Get deepest underlying [`DataFusionError`] | ||
/// | ||
/// [`DataFusionError`]s sometimes form a chain, such as `DataFusionError::ArrowError()` in order to conform | ||
|
@@ -412,20 +415,34 @@ impl DataFusionError { | |
|
||
pub fn strip_backtrace(&self) -> String { | ||
self.to_string() | ||
.split("\n\nbacktrace: ") | ||
.split(Self::BACK_TRACE_SEP) | ||
.collect::<Vec<&str>>() | ||
.first() | ||
.unwrap_or(&"") | ||
.to_string() | ||
} | ||
|
||
/// To enable optional rust backtrace in DataFusion: | ||
/// - [`Setup Env Variables`]<https://doc.rust-lang.org/std/backtrace/index.html#environment-variables> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
/// - Enable `backtrace` cargo feature | ||
/// | ||
/// Example: | ||
/// cargo build --features 'backtrace' | ||
/// RUST_BACKTRACE=1 ./app | ||
#[inline(always)] | ||
pub fn get_back_trace() -> String { | ||
let back_trace = Backtrace::capture(); | ||
if back_trace.status() == BacktraceStatus::Captured { | ||
return format!("\n\nbacktrace: {}", back_trace); | ||
#[cfg(feature = "backtrace")] | ||
{ | ||
let back_trace = Backtrace::capture(); | ||
if back_trace.status() == BacktraceStatus::Captured { | ||
return format!("{}{}", Self::BACK_TRACE_SEP, back_trace); | ||
} | ||
|
||
"".to_owned() | ||
} | ||
|
||
"".to_string() | ||
#[cfg(not(feature = "backtrace"))] | ||
"".to_owned() | ||
} | ||
} | ||
|
||
|
@@ -522,6 +539,41 @@ mod test { | |
assert_eq!(res.strip_backtrace(), "Arrow error: Schema error: bar"); | ||
} | ||
|
||
// RUST_BACKTRACE=1 cargo test --features backtrace --package datafusion-common --lib -- error::test::test_backtrace | ||
#[cfg(feature = "backtrace")] | ||
#[test] | ||
#[allow(clippy::unnecessary_literal_unwrap)] | ||
fn test_enabled_backtrace() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should also add a CI test, perhaps like https://github.com/apache/arrow-datafusion/blob/cde74016e930ffd9c55eed403b84bcd026f38d0f/.github/workflows/rust.yml#L99 |
||
let res: Result<(), DataFusionError> = plan_err!("Err"); | ||
let err = res.unwrap_err().to_string(); | ||
assert!(err.contains(DataFusionError::BACK_TRACE_SEP)); | ||
assert_eq!( | ||
err.split(DataFusionError::BACK_TRACE_SEP) | ||
.collect::<Vec<&str>>() | ||
.get(0) | ||
.unwrap(), | ||
&"Error during planning: Err" | ||
); | ||
assert!( | ||
err.split(DataFusionError::BACK_TRACE_SEP) | ||
.collect::<Vec<&str>>() | ||
.get(1) | ||
.unwrap() | ||
.len() | ||
> 0 | ||
); | ||
} | ||
|
||
#[cfg(not(feature = "backtrace"))] | ||
#[test] | ||
#[allow(clippy::unnecessary_literal_unwrap)] | ||
fn test_disabled_backtrace() { | ||
let res: Result<(), DataFusionError> = plan_err!("Err"); | ||
let res = res.unwrap_err().to_string(); | ||
assert!(!res.contains(DataFusionError::BACK_TRACE_SEP)); | ||
assert_eq!(res, "Error during planning: Err"); | ||
} | ||
|
||
#[test] | ||
fn test_find_root_error() { | ||
do_root_test( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As written, to be enabled, a user would have to use
datafusion_common
directly (they couldn't activate it throughdatafusion
)To enable it via datafusion we also need to add it to core/Cargo.toml, like this:
https://github.com/apache/arrow-datafusion/blob/cde74016e930ffd9c55eed403b84bcd026f38d0f/datafusion/core/Cargo.toml#L44
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I was little bit struggling on how it make on the datafusion level