GH-46704: [C++] Fix OSS-Fuzz build failure #46706
Merged
Conversation
This file contains hidden or 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
PR apache#46408 changed by mistake list-view IPC tests to use the same data as list tests. This was detected as a duplicate corpus file by the OSS-Fuzz CI build. This PR also includes a fix for a regression in the CUDA tests, due to reading non-CPU memory.
Member
Author
|
@github-actions crossbow submit -g cpp |
|
Revision: 7ee42d1 Submitted crossbow builds: ursacomputing/crossbow @ actions-4d70c06e41
|
Member
Author
|
@brunal FYI. I should have caught this during code review :) |
Contributor
|
Thank you for cleaning up after me! |
kou
reviewed
Jun 4, 2025
github-actions
bot
added
awaiting changes
Member
Author
|
Any other comments @kou ? |
github-actions
bot
added
awaiting merge
zanmato1984
reviewed
Jun 6, 2025
| const int64_t required_bytes = sizeof(offset_type) * (array.length() + 1); | ||
|
|
||
| offset_type first_offset = 0; | ||
| RETURN_NOT_OK(MemoryManager::CopyBufferSliceToCPU( |
Contributor
There was a problem hiding this comment.
I think I'll need some context here. Is this function supposed to be able to deal with non-CPU arrays? Does this rule apply to other functions in this source file too?
Comment on lines
+342
to
+344
| if (!array.data()->buffers[1]->is_cpu()) { | ||
| return Status::NotImplemented("Rebasing non-CPU offsets"); | ||
| } |
Contributor
There was a problem hiding this comment.
Two questions:
- Is this error handling necessary to fix the "CUDA tests" mentioned in the PR description?
- It appears that if the CUDA tests were failing with accessing the non-CPU
first_offset, and the aboveMemoryManager::CopyBufferSliceToCPUis supposed to fix it, wouldn't this check fail the tests again withUnimplemented? (I assume the said tests are exercising non-CPU buffers withfirst_offset > 0.)
Member
Author
There was a problem hiding this comment.
- Yes
- This codepath is only taken with a non-zero first offset, but the CUDA tests don't exercise that (otherwise they would have been crashing on git main too)
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 5f7d885. There were 65 benchmark results with an error:
There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them. |
alinaliBQ
pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
Jun 17, 2025
### Rationale for this change PR apache#46408 included a typo that changed list-view IPC tests to use the same data as list tests. This was detected as a duplicate corpus file by the OSS-Fuzz CI build. ### What changes are included in this PR? Undo mistake that led to using the same test data for lists and list-views. Also fix a regression in the CUDA tests, due to reading non-CPU memory when fetching the first offset in a list/binary array. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#46704 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Rationale for this change
PR #46408 included a typo that changed list-view IPC tests to use the same data as list tests. This was detected as a duplicate corpus file by the OSS-Fuzz CI build.
What changes are included in this PR?
Undo mistake that led to using the same test data for lists and list-views. Also fix a regression in the CUDA tests, due to reading non-CPU memory when fetching the first offset in a list/binary array.
Are these changes tested?
Yes.
Are there any user-facing changes?
No.