feat: enable debug assertions in CI profile, fix unaligned memory access bug#3652
Merged
andygrove merged 5 commits intoapache:mainfrom Mar 10, 2026
Merged
feat: enable debug assertions in CI profile, fix unaligned memory access bug#3652andygrove merged 5 commits intoapache:mainfrom
andygrove merged 5 commits intoapache:mainfrom
Conversation
…kflows Enable debug-assertions in the CI cargo profile to catch potential issues with unsafe code. Enable RUST_BACKTRACE=1 in all CI workflows to get full stack traces when panics occur.
andygrove
changed the title
Log panic messages (including debug assertion text and location) to stderr in the panic hook so they are visible in CI logs. Wrap the spawned tokio execution task with catch_unwind so panics propagate as DataFusionError instead of silently dropping the channel.
Member
Author
|
Spark's UnsafeRow data from JVM may not be 8-byte aligned. The null bitset accessors were casting to *const i64 and dereferencing directly, which triggers a panic with debug assertions enabled. Use read_unaligned/write_unaligned instead.
andygrove
changed the title
Member
Author
|
@mbutrovich I think this explains the bus error you saw recently |
andygrove
commented
Mar 10, 2026
| while let Some(batch) = stream.next().await { | ||
| if tx.send(batch).await.is_err() { | ||
| break; | ||
| let result = std::panic::AssertUnwindSafe(async { |
Member
Author
There was a problem hiding this comment.
These changes are for catching panics that happen in tokio threads
andygrove
commented
Mar 10, 2026
Comment on lines
+149
to
+151
| // Log the panic message and location to stderr so it is visible in CI logs | ||
| // even if the exception message is lost crossing the FFI boundary | ||
| eprintln!("Comet native panic: {panic_info}"); |
Member
Author
There was a problem hiding this comment.
This is the main change for improving reporting of panics
parthchandra
approved these changes
Mar 10, 2026
Contributor
parthchandra
left a comment
parthchandra
left a comment
There was a problem hiding this comment.
Let's get this in. It's important to catch these issues early.
Member
Author
|
Thanks @parthchandra |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3653
Summary
debug-assertions = truein the CI cargo profile to catch potential issues with unsafe code at test timeRUST_BACKTRACE=1in all CI workflows to get full stack traces when panics occurRationale
We want to verify whether any debug assertions fire on the main branch, particularly in the columnar-to-row and other unsafe code paths.