Implement compare operations for view types #5900
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alamb
previously approved these changes
Jun 17, 2024
There was a problem hiding this comment.
Unfortunately, this PR becomes larger than I thought, partly because the test cases are rather comprehensive and the comparison is non-trivial.
👍
This PR is really nicely done in my opinion @XiangpengHao 🏆
The only things I think are required prior to merging is some additional test coverage (I left a note about what I think i smissing)
I also left some suggestions that might make the code easier to read, but I don't think they are necessary
Thanks again
cc @ariesdevil and @Weijun-H
| @@ -538,6 +540,135 @@ impl<'a, T: ByteArrayType> ArrayOrd for &'a GenericByteArray<T> { | |||
| } | |||
| } | |||
|
|
|||
| /// Comparing two ByteView types are non-trivial. | |||
| /// It takes a bit of patience to understand why we don't just compare two &[u8] directly. | |||
alamb
dismissed
their stale review
Need some additional test coverage prior to merging
|
I also filed #5903 to track adding benchmarks for these kernels |
|
Added more tests and refactored a bit to make it more clear! can you take a look again? @alamb |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #5897 .
Rationale for this change
Now we can compare two view type arrays.
Unfortunately, this PR becomes larger than I thought, partly because the test cases are rather comprehensive and the comparison is non-trivial.
The view type comparison code might be slightly more esoteric than one might expected, I hope the comments could help understanding/justify the complexity.
What changes are included in this PR?
Are there any user-facing changes?