You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/06/30 16:28:05 UTC
[GitHub] [arrow-rs] bjchambers opened a new issue #514: Struct equality on slices has false negatives
bjchambers opened a new issue #514:
URL: https://github.com/apache/arrow-rs/issues/514
**Describe the bug**
A struct array that is sliced to a subset is not equal to a struct array created from just that subset of data. Stepping through the debugger I *think* it is because some of the null/value comparisons drop the offset and end up comparing the wrong part of the array.
**To Reproduce**
Failing test case in the most recent commit on this branch:
https://github.com/bjchambers/arrow-rs/tree/struct-slice-equality
**Expected behavior**
Struct array slices compare equal to equivalent struct arrays.
Or, the printing of the struct arrays shows how they differ.
--
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] alamb commented on issue #514: Struct equality on slices has false negatives
Posted by GitBox <gi...@apache.org>.
alamb commented on issue #514:
URL: https://github.com/apache/arrow-rs/issues/514#issuecomment-916249592
@bjchambers -- https://github.com/apache/arrow-rs/pull/691#discussion_r705493693 fixed something related to equality of lists, perhaps it is related to this one as well
--
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] bjchambers commented on issue #514: Struct equality on slices has false negatives
Posted by GitBox <gi...@apache.org>.
bjchambers commented on issue #514:
URL: https://github.com/apache/arrow-rs/issues/514#issuecomment-883792723
Hrm, upon closer inspection, that didn't fix this.
The tests are still failing, and I think I'm outside my depth debugging. It looks like something isn't lining up with the null bits or the null counts in the sliced structs.
@nevi-me do you have any ideas on what may be failing, or suggestions for how to find t he problems? I don't have a strong enough grasp on how it's expected to work, so it's unclear what is wrong when I'm debugging. Any hints would be helpful.
--
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] bjchambers commented on issue #514: Struct equality on slices has false negatives
Posted by GitBox <gi...@apache.org>.
bjchambers commented on issue #514:
URL: https://github.com/apache/arrow-rs/issues/514#issuecomment-873427233
I think the problem here may be more widespread than I originall thought. It looks like the equality methods are inconsistent in how they handle offset. For instance:
```
pub fn equal(lhs: &ArrayData, rhs: &ArrayData) -> bool {
let lhs_nulls = lhs.null_buffer();
let rhs_nulls = rhs.null_buffer();
utils::base_equal(lhs, rhs)
&& lhs.null_count() == rhs.null_count()
&& utils::equal_nulls(lhs, rhs, lhs_nulls, rhs_nulls, 0, 0, lhs.len())
&& equal_values(lhs, rhs, lhs_nulls, rhs_nulls, 0, 0, lhs.len())
}
```
This starts has `lhs_start` and `rhs_start` as 0. This would suggest that `equal_nulls` and `equal_values` should add the start to the offset.
```
#[inline]
pub(super) fn equal_nulls(
lhs: &ArrayData,
rhs: &ArrayData,
lhs_nulls: Option<&Buffer>,
rhs_nulls: Option<&Buffer>,
lhs_start: usize,
rhs_start: usize,
len: usize,
) -> bool {
let lhs_null_count = count_nulls(lhs_nulls, lhs_start, len);
let rhs_null_count = count_nulls(rhs_nulls, rhs_start, len);
if lhs_null_count > 0 || rhs_null_count > 0 {
let lhs_values = lhs_nulls.unwrap().as_slice();
let rhs_values = rhs_nulls.unwrap().as_slice();
equal_bits(
lhs_values,
rhs_values,
lhs_start + lhs.offset(),
rhs_start + rhs.offset(),
len,
)
} else {
true
}
}
```
This uses the `lhs_start` directly for counting nulls (which doesn't add the offset), but then adds the offset in when calling `equal_bits`. Looking at other parts of equality (such as boolean, primitives, etc.) it seems that it is inconsistent as to whether the offset should be included or not in the start values.
I can a stab at fixing it for at least the case identified, but I'm somewhat curious what the intended approach is. It seems like there are two options:
1. The `lhs_start` and `rhs_start` already include the offset, so none of the helper methods should add it again. This seems like it could include the offset just once (up front) and then everything else wouldn't need to, but it may run into more problems if some of the helper methods do include the offset (eg., running off the end of the array, etc.). Also may make it harder to use the `lhs_start` and `rhs_start` with public methods on the array data that respect offset.
2. The `lhs_start` and `rhs_start` are always relative the corresponding offset. When indexing into the data using methods that aren't relative the offset, it needs to be added.
It seems like option 2 is most consistent with how things are currently implemented.
--
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] bjchambers commented on issue #514: Struct equality on slices has false negatives
Posted by GitBox <gi...@apache.org>.
bjchambers commented on issue #514:
URL: https://github.com/apache/arrow-rs/issues/514#issuecomment-880371136
I think that #389 seems to have fixed this. I'll make a PR with the corresponding test case to prevent regression.
--
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