You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "albel727 (via GitHub)" <gi...@apache.org> on 2023/01/20 23:57:10 UTC
[GitHub] [arrow-rs] albel727 opened a new issue, #3579: `nullif` incorrectly calculates `null_count`, sometimes panics with substraction overflow error
albel727 opened a new issue, #3579:
URL: https://github.com/apache/arrow-rs/issues/3579
**Describe the bug**
`nullif(left, right)` incorrectly calculates `null_count` for the result array, whenever `left` doesn't have a null_buffer and has `len % 64 == 0`. It can even panic, if there are less than 64 true values in `right`.
**To Reproduce**
```rust
let n = 64;
let left = Int32Array::from((0..n as i32).into_iter().collect::<Vec<_>>());
let right = BooleanArray::from((0..n).into_iter().map(|_| None).collect::<Vec<_>>());
arrow_select::nullif::nullif(&left, &right);
```
**Expected behavior**
It works.
**Actual behavior**
It panics with 'attempt to subtract with overflow' at this line:
https://github.com/apache/arrow-rs/blob/796b670338ce33806a39777ea18cf6fae8fa7ee4/arrow-select/src/nullif.rs#L107
**Additional context**
The problem evaded fixes #3034 and #3263, which were incomplete. The wrong calculation occurs in these lines:
https://github.com/apache/arrow-rs/blob/796b670338ce33806a39777ea18cf6fae8fa7ee4/arrow-select/src/nullif.rs#L81-L90
The reason it is wrong is the un-intuitive, if not to say wrong, behavior of `bitwise_unary_op_helper()` and friends. They're calling `op()` **unconditionally** on the remainder bits, even if there are none:
https://github.com/apache/arrow-rs/blob/3a90654f4cb98ac7fe278991a6cbc11384664e2e/arrow-buffer/src/buffer/ops.rs#L119-L123
It doesn't make a difference for the produced output buffer, because it is then just extended with slice of 0 bytes, i.e. remains unaffected. But it does matter for FnMut closures with side effects, such as the one found in `nullif`, which, as a result of this behavior, adds an excess of 64 to `valid_count`, when there are no remainder bits, i.e. bit length is a multiple of 64.
The quick fix is to do `valid_count -= 64 - remainder_len;` in `nullif()` unconditionally, i.e. just remove the `if remainder_len != 0 {` condition around it. It was clearly written in assumption, that `bitwise_unary_op_helper()` doesn't call `op()` in excess, when there are no remainder bits.
A better fix could have been to avoid making excess `op()` calls in `bitwise_unary_op_helper()` and friends, but since they're public, it could lead to bugs in external consumers, which might have come to rely on this weird behavior.
In any case, I suggest at least checking whether other usages of `bitwise_unary_op_helper()` and friends
in arrow-rs make the same incorrect assumption. Temporarily changing the type of `op` parameter from `FnMut` to `Fn` and looking at the compilation error fallout should point out all suspicious places.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold closed issue #3579: `nullif` incorrectly calculates `null_count`, sometimes panics with substraction overflow error
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold closed issue #3579: `nullif` incorrectly calculates `null_count`, sometimes panics with substraction overflow error
URL: https://github.com/apache/arrow-rs/issues/3579
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on issue #3579: `nullif` incorrectly calculates `null_count`, sometimes panics with substraction overflow error
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on issue #3579:
URL: https://github.com/apache/arrow-rs/issues/3579#issuecomment-1399223318
Thank you for the detailed report, makes sense to me. The change to FnMut was relatively recent so I think this should be the only impacted location, but I will verify.
Bit embarrassing this kernel has taken quite so many iterations to fix 😅
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on issue #3579: `nullif` incorrectly calculates `null_count`, sometimes panics with substraction overflow error
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on issue #3579:
URL: https://github.com/apache/arrow-rs/issues/3579#issuecomment-1406713279
`label_issue.py` automatically added labels {'arrow'} from #3590
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on issue #3579: `nullif` incorrectly calculates `null_count`, sometimes panics with substraction overflow error
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on issue #3579:
URL: https://github.com/apache/arrow-rs/issues/3579#issuecomment-1400852122
A potential bug I found whilst testing this, dating from 2020 - https://github.com/apache/arrow-rs/issues/3589
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] albel727 commented on issue #3579: `nullif` incorrectly calculates `null_count`, sometimes panics with substraction overflow error
Posted by "albel727 (via GitHub)" <gi...@apache.org>.
albel727 commented on issue #3579:
URL: https://github.com/apache/arrow-rs/issues/3579#issuecomment-1399086004
Just in case, here's the code that I used to validate that the quick fix works:
```rust
std::panic::set_hook(Box::new(|_info| { /* silence panics */ }));
for n in 0..=128+64 {
let left = Int32Array::from((0..n as i32).into_iter().collect::<Vec<_>>());
for somes in 0..=n {
for trues in 0..=somes {
let right = BooleanArray::from((0..n).into_iter().map(|i| {
Some(i < trues).filter(|_| i < somes)
}).collect::<Vec<_>>());
let ok = std::panic::catch_unwind(|| {
let arr = arrow_select::nullif::nullif(&left, &right).unwrap();
let arr: &Int32Array = arrow_array::cast::as_primitive_array(&arr);
arrow_array::Array::data(arr).validate_full().unwrap();
}).is_ok();
if !ok {
println!("{n} {somes} {trues}");
}
}
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org