pitrou commented on code in PR #39772:
URL: https://github.com/apache/arrow/pull/39772#discussion_r1471496603
##########
cpp/src/arrow/array/data.h:
##########
@@ -36,6 +36,7 @@ namespace arrow {
class Array;
struct ArrayData;
+class MemoryManager;
Review Comment:
Please let's include `arrow/type_fwd.h` instead.
##########
cpp/src/arrow/array/array_base.h:
##########
@@ -165,6 +166,28 @@ class ARROW_EXPORT Array {
/// An error is returned if the types are not layout-compatible.
Result<std::shared_ptr<Array>> View(const std::shared_ptr<DataType>& type)
const;
+ /// \brief Construct a copy of the array with all buffers on destination
+ /// Memory Manager
+ ///
+ /// This method recursively copies the array's buffers and those of its
children
+ /// onto the destination MemoryManager device and returns the new Array.
+ Result<std::shared_ptr<Array>> CopyTo(const std::shared_ptr<MemoryManager>&
to) const {
+ ARROW_ASSIGN_OR_RAISE(auto copied_data, data()->CopyTo(to));
Review Comment:
I don't think there's a reason to inline this function definition in the
`.h` file. By moving it into the corresponding `.cc`, we can cut down on an
additional header inclusion.
##########
cpp/src/arrow/record_batch.h:
##########
@@ -32,6 +32,8 @@
namespace arrow {
+class MemoryManager;
Review Comment:
Same here: use `arrow/type_fwd.h`
##########
cpp/src/arrow/array/data.cc:
##########
@@ -140,6 +141,49 @@ std::shared_ptr<ArrayData>
ArrayData::Make(std::shared_ptr<DataType> type, int64
return std::make_shared<ArrayData>(std::move(type), length, null_count,
offset);
}
+namespace {
+template <typename Fn>
+Result<std::shared_ptr<ArrayData>> copy_to_impl(const ArrayData& data,
+ const
std::shared_ptr<MemoryManager>& to,
+ Fn&& copy_fn) {
+ auto output = ArrayData::Make(data.type, data.length, data.null_count,
data.offset);
+ output->buffers.reserve(data.buffers.size());
+
+ for (const auto& buf : data.buffers) {
+ if (!buf) {
+ output->buffers.push_back(nullptr);
+ continue;
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto temp_buf, copy_fn(buf, to));
+ output->buffers.push_back(std::move(temp_buf));
+ }
Review Comment:
This is fine, but this can probably be shortened to something like:
```suggestion
output->buffers.resize(data.buffers.size());
for (auto& [buf, out_buf] : Zip(data.buffers, output->buffers) {
if (buf) {
ARROW_ASSIGN_OR_RAISE(out_buf, copy_fn(buf, to));
}
}
```
(you need `arrow/util/range.h` for the `Zip` construct)
--
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]
