Skip to content
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

chore: Add Miri workflow #636

Merged
merged 9 commits into from
Jul 11, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions .github/workflows/miri.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.

name: Run Miri Safety Checks

on:
push:
paths-ignore:
- "doc/**"
- "docs/**"
- "**.md"
pull_request:
paths-ignore:
- "doc/**"
- "docs/**"
- "**.md"
# manual trigger
# https://docs.github.com/en/actions/managing-workflow-runs/manually-running-a-workflow
workflow_dispatch:

jobs:
miri:
name: "Miri"
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Install Miri
run: |
rustup toolchain install nightly --component miri
rustup override set nightly
cargo miri setup
- name: Test with Miri
run: |
cd core
MIRIFLAGS="-Zmiri-disable-isolation" cargo miri test
1 change: 1 addition & 0 deletions core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ twox-hash = "1.6.3"

[features]
default = []
nightly = []
Copy link
Member Author

Choose a reason for hiding this comment

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

We have some references to this feature in our code and get warnings when we run miri (which requires use of nightly Rust)


[profile.release]
debug = true
Expand Down
9 changes: 9 additions & 0 deletions core/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,7 @@ mod tests {
}

#[test]
#[cfg_attr(miri, ignore)] // miri can't call foreign function `dlopen`
pub fn error_from_panic() {
let _guard = attach_current_thread();
let mut env = jvm().get_env().unwrap();
Expand All @@ -604,6 +605,7 @@ mod tests {
// Verify that functions that return an object are handled correctly. This is basically
// a test of the "happy path".
#[test]
#[cfg_attr(miri, ignore)] // miri can't call foreign function `dlopen`
pub fn object_result() {
let _guard = attach_current_thread();
let mut env = jvm().get_env().unwrap();
Expand All @@ -621,6 +623,7 @@ mod tests {
// Verify that functions that return an native time are handled correctly. This is basically
// a test of the "happy path".
#[test]
#[cfg_attr(miri, ignore)] // miri can't call foreign function `dlopen`
pub fn jlong_result() {
let _guard = attach_current_thread();
let mut env = jvm().get_env().unwrap();
Expand All @@ -637,6 +640,7 @@ mod tests {
// Verify that functions that return an array can handle throwing exceptions. The test
// causes an exception by dividing by zero.
#[test]
#[cfg_attr(miri, ignore)] // miri can't call foreign function `dlopen`
pub fn jlong_panic_exception() {
let _guard = attach_current_thread();
let mut env = jvm().get_env().unwrap();
Expand All @@ -657,6 +661,7 @@ mod tests {
// Verify that functions that return an native time are handled correctly. This is basically
// a test of the "happy path".
#[test]
#[cfg_attr(miri, ignore)] // miri can't call foreign function `dlopen`
pub fn jlong_result_ok() {
let _guard = attach_current_thread();
let mut env = jvm().get_env().unwrap();
Expand All @@ -673,6 +678,7 @@ mod tests {
// Verify that functions that return an native time are handled correctly. This is basically
// a test of the "happy path".
#[test]
#[cfg_attr(miri, ignore)] // miri can't call foreign function `dlopen`
pub fn jlong_result_err() {
let _guard = attach_current_thread();
let mut env = jvm().get_env().unwrap();
Expand All @@ -693,6 +699,7 @@ mod tests {
// Verify that functions that return an array are handled correctly. This is basically
// a test of the "happy path".
#[test]
#[cfg_attr(miri, ignore)] // miri can't call foreign function `dlopen`
pub fn jint_array_result() {
let _guard = attach_current_thread();
let mut env = jvm().get_env().unwrap();
Expand All @@ -713,6 +720,7 @@ mod tests {
// Verify that functions that return an array can handle throwing exceptions. The test
// causes an exception by dividing by zero.
#[test]
#[cfg_attr(miri, ignore)] // miri can't call foreign function `dlopen`
pub fn jint_array_panic_exception() {
let _guard = attach_current_thread();
let mut env = jvm().get_env().unwrap();
Expand All @@ -736,6 +744,7 @@ mod tests {
/// See [`object_panic_exception`] for a test which involves generating a panic and verifying
/// that the resulting stack trace includes the offending call.
#[test]
#[cfg_attr(miri, ignore)] // miri can't call foreign function `dlopen`
pub fn stacktrace_string() {
// Setup: Start with a backtrace that includes all of the expected scenarios, including
// cases where the file and location are not provided as part of the backtrace capture
Expand Down
2 changes: 2 additions & 0 deletions core/src/execution/datafusion/expressions/cast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1646,6 +1646,7 @@ mod tests {
use super::*;

#[test]
#[cfg_attr(miri, ignore)] // test takes too long with miri
fn timestamp_parser_test() {
// write for all formats
assert_eq!(
Expand Down Expand Up @@ -1683,6 +1684,7 @@ mod tests {
}

#[test]
#[cfg_attr(miri, ignore)] // test takes too long with miri
fn test_cast_string_to_timestamp() {
let array: ArrayRef = Arc::new(StringArray::from(vec![
Some("2020-01-01T12:34:56.123456"),
Expand Down
1 change: 1 addition & 0 deletions core/src/execution/datafusion/expressions/xxhash64.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ mod test {
use twox_hash::XxHash64;

#[test]
#[cfg_attr(miri, ignore)] // test takes too long with miri
fn test_xxhash64_random() {
let mut rng = rand::thread_rng();
for len in 0..128 {
Expand Down
1 change: 1 addition & 0 deletions core/src/execution/datafusion/shuffle_writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1447,6 +1447,7 @@ mod test {
}

#[test]
#[cfg_attr(miri, ignore)] // miri can't call foreign function `ZSTD_createCCtx`
fn test_insert_larger_batch() {
let schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Utf8, true)]));
let mut b = StringBuilder::new();
Expand Down
5 changes: 1 addition & 4 deletions core/src/execution/datafusion/spark_hash.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,7 @@ pub(crate) fn spark_compatible_murmur3_hash<T: AsRef<[u8]>>(data: T, seed: u32)
// data is &[u8] so we do not need to check for proper alignment
unsafe {
let mut h1 = if len_aligned > 0 {
hash_bytes_by_int(
std::slice::from_raw_parts(data.get_unchecked(0), len_aligned),
seed,
)
hash_bytes_by_int(&data[0..len_aligned], seed)
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes a miri warning. I ran the murmur3 bench before and after this change and did not see a significant difference.

} else {
seed as i32
};
Expand Down
4 changes: 4 additions & 0 deletions core/src/execution/kernels/temporal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -824,6 +824,7 @@ mod tests {
use std::sync::Arc;

#[test]
#[cfg_attr(miri, ignore)] // test takes too long with miri
fn test_date_trunc() {
let size = 1000;
let mut vec: Vec<i32> = Vec::with_capacity(size);
Expand Down Expand Up @@ -962,6 +963,7 @@ mod tests {
}

#[test]
#[cfg_attr(miri, ignore)] // test takes too long with miri
fn test_timestamp_trunc() {
let size = 1000;
let mut vec: Vec<i64> = Vec::with_capacity(size);
Expand Down Expand Up @@ -998,6 +1000,8 @@ mod tests {
}

#[test]
// test takes too long with miri
#[cfg_attr(miri, ignore)]
// This test only verifies that the various input array types work. Actually correctness to
// ensure this produces the same results as spark is verified in the JVM tests
fn test_timestamp_trunc_array_fmt_dyn() {
Expand Down
2 changes: 1 addition & 1 deletion core/src/execution/sort.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes a miri warning

bucket.len(),
);
}
Expand Down
Loading