Skip to content

ARROW-8598: [Rust] simd_compare_op creates buffer of incorrect length #7043

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.

Sign up for GitHub

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

Closed
paddyhoran wants to merge 3 commits into from
Closed

ARROW-8598: [Rust] simd_compare_op creates buffer of incorrect length #7043

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
473ef40
Added test to re-create issue.
paddyhoran Apr 27, 2020
a52907c
Added fix for `simd_compare_op`
paddyhoran Apr 27, 2020
2d3328f
Fix for case where `len` is a multiple of `lanes.`
paddyhoran Apr 27, 2020
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
37 changes: 34 additions & 3 deletions rust/arrow/src/compute/kernels/comparison.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,8 @@ where
T: ArrowNumericType,
F: Fn(T::Simd, T::Simd) -> T::SimdMask,
{
if left.len() != right.len() {
let len = left.len();
if len != right.len() {
return Err(ArrowError::ComputeError(
"Cannot perform comparison operation on arrays of different length"
.to_string(),
Expand All @@ -224,9 +225,11 @@ where
)?;

let lanes = T::lanes();
let mut result = BooleanBufferBuilder::new(left.len());
let mut result = BooleanBufferBuilder::new(len);

for i in (0..left.len()).step_by(lanes) {
let rem = len % lanes;

for i in (0..len - rem).step_by(lanes) {
let simd_left = T::load(left.value_slice(i, lanes));
let simd_right = T::load(right.value_slice(i, lanes));
let simd_result = op(simd_left, simd_right);
Expand All @@ -235,6 +238,15 @@ where
}
}

if rem > 0 {
let simd_left = T::load(left.value_slice(len - rem, lanes));
let simd_right = T::load(right.value_slice(len - rem, lanes));
let simd_result = op(simd_left, simd_right);
for i in 0..rem {
result.append(T::mask_get(&simd_result, i))?;
}
}

let data = ArrayData::new(
DataType::Boolean,
left.len(),
Expand Down Expand Up @@ -351,6 +363,7 @@ where
mod tests {
use super::*;
use crate::array::Int32Array;
use crate::datatypes::Int8Type;

#[test]
fn test_primitive_array_eq() {
Expand Down Expand Up @@ -464,6 +477,24 @@ mod tests {
assert_eq!(true, c.value(2));
}

#[test]
fn test_length_of_result_buffer() {
// `item_count` is chosen to not be a multiple of the number of SIMD lanes for this
// type (`Int8Type`), 64.
let item_count = 130;

let select_mask: BooleanArray = vec![true; item_count].into();

let array_a: PrimitiveArray<Int8Type> = vec![1; item_count].into();
let array_b: PrimitiveArray<Int8Type> = vec![2; item_count].into();
let result_mask = gt_eq(&array_a, &array_b).unwrap();

assert_eq!(
result_mask.data().buffers()[0].len(),
select_mask.data().buffers()[0].len()
);
}

macro_rules! test_utf8 {
($test_name:ident, $left:expr, $right:expr, $op:expr, $expected:expr) => {
#[test]
Expand Down