Skip to content

Conversation

@comphead
Copy link
Contributor

Which issue does this PR close?

Closes #7522.

Rationale for this change

In #7434 the backtrace was introduced, however its expensive to build and should be optional. Introducing a cargo feature for backtrace

What changes are included in this PR?

Are these changes tested?

yes

Are there any user-facing changes?

@comphead
Copy link
Contributor Author

@alamb @crepererum

Copy link
Contributor

@crepererum crepererum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds a build config, not a cargo feature. A cargo feature (which I think should be preferred because build configs are an even bigger mess) works like this:

  1. add feature to Cargo.toml
  2. change the cfg guard to #[cfg(feature = "backtrace")]

Also see https://doc.rust-lang.org/cargo/reference/features.html.

On top of that, I think this needs some docs, at least for get_back_trace (you probably want to use a single method so you can easily reuse the docstring and either use the cfg-guards within that method or create a new private fn called get_back_trace_impl).

@comphead comphead marked this pull request as ready for review September 13, 2023 18:20
@comphead comphead requested a review from crepererum September 13, 2023 18:20
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @comphead -- I think this is looking very close

}

/// To enable optional rust backtrace in DataFusion:
/// - [`Setup Env Variables`]<https://doc.rust-lang.org/std/backtrace/index.html#environment-variables>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

path = "src/lib.rs"

[features]
avro = ["apache-avro"]
Copy link
Contributor

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 through datafusion)

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

Copy link
Contributor Author

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

#[cfg(feature = "backtrace")]
#[test]
#[allow(clippy::unnecessary_literal_unwrap)]
fn test_enabled_backtrace() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@github-actions github-actions bot added the core Core DataFusion crate label Sep 13, 2023
@comphead
Copy link
Contributor Author

2 tests failed because the configuration is different between architectures for
amd:

 env:
    RUSTFLAGS: -C debuginfo=1
    CARGO_INCREMENTAL: 0
    RUST_BACKTRACE: 1

mac,win

  env:
    RUSTFLAGS: -C debuginfo=0

Lack of BACKTRACE=1 on mac,win arch expectedly lead the test to fail. @alamb would you mind to help where this set up?

@comphead comphead requested a review from alamb September 14, 2023 16:39
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lack of BACKTRACE=1 on mac,win arch expectedly lead the test to fail. @alamb would you mind to help where this set up?

It looks like you have solved this already @comphead -- let me know if there is something else you are looking for

Otherwise I think this PR is looking ready to go. Thank you 🙏

@alamb alamb merged commit 1a986c2 into apache:main Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Performance Regression: Backtraces in errors slow down planning time (Expensive backtraces)

3 participants