zanmato1984 commented on code in PR #46706:
URL: https://github.com/apache/arrow/pull/46706#discussion_r2132900107
##########
cpp/src/arrow/ipc/writer.cc:
##########
@@ -329,15 +329,24 @@ class RecordBatchSerializer {
return Status::OK();
}
- int64_t required_bytes = sizeof(offset_type) * (array.length() + 1);
- if (array.value_offset(0) > 0) {
+ const int64_t required_bytes = sizeof(offset_type) * (array.length() + 1);
+
+ offset_type first_offset = 0;
+ RETURN_NOT_OK(MemoryManager::CopyBufferSliceToCPU(
+ array.data()->buffers[1], array.offset() * sizeof(offset_type),
+ sizeof(offset_type), reinterpret_cast<uint8_t*>(&first_offset)));
+
+ if (first_offset > 0) {
// If the offset of the first value is non-zero, then we must create a
new
// offsets buffer with shifted offsets.
+ if (!array.data()->buffers[1]->is_cpu()) {
+ return Status::NotImplemented("Rebasing non-CPU offsets");
+ }
Review Comment:
Two questions:
1. Is this error handling necessary to fix the "CUDA tests" mentioned in the
PR description?
2. It appears that if the CUDA tests were failing with accessing the non-CPU
`first_offset`, and the above `MemoryManager::CopyBufferSliceToCPU` is supposed
to fix it, wouldn't this check fail the tests again with `Unimplemented`? (I
assume the said tests are exercising non-CPU buffers with `first_offset > 0`.)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
