-
Notifications
You must be signed in to change notification settings - Fork 847
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
feat: add ListView equal #6969
base: main
Are you sure you want to change the base?
feat: add ListView equal #6969
Conversation
if lhs_size != rhs_size { | ||
return false; | ||
} | ||
|
||
// check if null | ||
if let (Some(lhs_null), Some(rhs_null)) = (lhs_nulls, rhs_nulls) { | ||
if lhs_null.is_null(lhs_pos) != rhs_null.is_null(rhs_pos) { | ||
return false; | ||
} | ||
if lhs_null.is_null(lhs_pos) { | ||
continue; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was taking a very close look at this code and I noticed two things:
Firstly, I believe having the size check before the nullability check can cause a bug in the following case:
#[test]
fn test_chuke() {
let data = Arc::new(Int32Array::from(vec![1]));
let field = Arc::new(Field::new("chuke", DataType::Int32, true));
let a = ListViewArray::try_new(
field.clone(),
vec![0, 0].into(), // offsets
vec![1, 1].into(), // sizes
data.clone(),
Some(vec![false, true].into()),
)
.unwrap();
let b = ListViewArray::try_new(
field.clone(),
vec![0, 0].into(), // offsets
vec![0, 1].into(), // sizes
data.clone(),
Some(vec![false, true].into()),
)
.unwrap();
test_equal(&a, &b, true);
}
a
and b
are valid ListViewArray
s, where their sizes buffers differs in their first element. This shouldn't matter since the value is null anyway, but this code will check this size first and falsely report as being not equal even though both arrays have null for that slot. Hence test will fail.
Secondly, I wonder if it's more efficient to pull the if let (Some(lhs_null), Some(rhs_null)) = (lhs_nulls, rhs_nulls)
outside of the loop, considering this wouldn't change per iteration?
Which issue does this PR close?
Closes #5501 .
Rationale for this change
What changes are included in this PR?
Add GenericListViewArray equal
Are there any user-facing changes?