-
Notifications
You must be signed in to change notification settings - Fork 662
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
Fix equality of nested nullable FixedSizeBinary (#4637) #4670
Fix equality of nested nullable FixedSizeBinary (#4637) #4670
Conversation
@@ -80,7 +80,7 @@ pub(super) fn fixed_binary_equal( | |||
lhs_start + lhs_nulls.offset(), | |||
len, | |||
); | |||
let rhs_nulls = lhs.nulls().unwrap(); | |||
let rhs_nulls = rhs.nulls().unwrap(); |
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.
It is actually remarkably hard to construct a test case to exercise this, kudos to @kawadakk for finding one. The reason it is hard is at this point the null masks have already been checked for equality, so you need one side to have a different start offset from the other, such that when correctly offset the null buffers compare equal, but the underlying null buffers themselves are not actually equal
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.
…ed-nullable-fixed-size-binary-array
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.
love it ❤️
@@ -80,7 +80,7 @@ pub(super) fn fixed_binary_equal( | |||
lhs_start + lhs_nulls.offset(), | |||
len, | |||
); | |||
let rhs_nulls = lhs.nulls().unwrap(); | |||
let rhs_nulls = rhs.nulls().unwrap(); |
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.
Which issue does this PR close?
Closes #4637
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?