chore: Add Miri workflow #636
Merged
Conversation
This file contains 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
andygrove
commented
Jul 5, 2024
core/Cargo.toml
Outdated
| @@ -97,6 +97,7 @@ twox-hash = "1.6.3" | |||
|
|
|||
| [features] | |||
| default = [] | |||
| nightly = [] | |||
There was a problem hiding this comment.
We have some references to this feature in our code and get warnings when we run miri (which requires use of nightly Rust)
andygrove
commented
Jul 5, 2024
| std::slice::from_raw_parts(data.get_unchecked(0), len_aligned), | ||
| seed, | ||
| ) | ||
| hash_bytes_by_int(&data[0..len_aligned], seed) |
There was a problem hiding this comment.
This fixes a miri warning. I ran the murmur3 bench before and after this change and did not see a significant difference.
andygrove
commented
Jul 5, 2024
core/src/execution/sort.rs
Outdated
| @@ -165,7 +165,7 @@ where | |||
| // because they are defined as Vec<Vec<T>> | |||
| ptr::copy_nonoverlapping( | |||
| bucket.as_ptr(), | |||
| self.get_unchecked_mut(pos), | |||
| self.as_mut_ptr().add(pos), | |||
There was a problem hiding this comment.
This fixes a miri warning
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #636 +/- ##
============================================
+ Coverage 53.43% 53.47% +0.03%
Complexity 802 802
============================================
Files 106 106
Lines 10152 10173 +21
Branches 1911 1915 +4
============================================
+ Hits 5425 5440 +15
- Misses 3751 3755 +4
- Partials 976 978 +2 ☔ View full report in Codecov by Sentry. |
viirya
approved these changes
Jul 10, 2024
himadripal
pushed a commit
to himadripal/datafusion-comet
that referenced
this pull request
Sep 7, 2024
* add miri workflow * fix * ignore some tests when miri is enabled * fix one safety warning and ignore some slow tests * fmt * prepare for review * add comments * update to reflect change in directory name
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Which issue does this PR close?
Closes #634
Rationale for this change
We want to ensure that our unsafe code is safe
What changes are included in this PR?
Add new workflow to run Miri checks
How are these changes tested?