-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add schema_err!
error macros with optional backtrace
#8620
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
.github/workflows/rust.yml
Outdated
@@ -99,6 +99,11 @@ jobs: | |||
rust-version: stable | |||
- name: Run tests (excluding doctests) | |||
run: cargo test --lib --tests --bins --features avro,json,backtrace | |||
env: |
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.
Adding more stack as q54 keeps failing.
thread 'tpcds_physical_q54' has overflowed its stack
This PR shouldn't impact stack as the introduced SchemaError
second param is boxed and thus resides in heap, in stack there is only a pointer. However it is what is is, I'm happy to get the opinion from other contributors on this
@alamb would you mind reviewing? |
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.
Looks like an improvement to me -- thank you @comphead
a0fa96c
to
73b1f16
Compare
Here the problem is backtrace-rs regression in Rust lang |
Wow -- super sleuth 🕵️ |
Tbh, all kudos to @Jefffrey who ran tons of experiments to narrow down the problem when we were investigating windows CI slowness, and eventually he came to backtrace root cause. |
Which issue does this PR close?
Closes partially #7360.
Rationale for this change
Continue covering Datafusion Errors with error macros supporting optional backtrace
What changes are included in this PR?
introduced
schema_err!
andschema_datafusion_err!
macros with optional backtrace, to representErr(DatafusionError::SchemaError)
andDatafusionError::SchemaError(_)
Are these changes tested?
Yes
Are there any user-facing changes?
No