Copilot commented on code in PR #51122:
URL: https://github.com/apache/arrow/pull/51122#discussion_r3951149114
##########
cpp/src/arrow/buffer.cc:
##########
@@ -148,22 +148,13 @@ Result<std::shared_ptr<Buffer>> Buffer::ViewOrCopy(
return MemoryManager::CopyBuffer(source, to);
}
-class StlStringBuffer : public Buffer {
- public:
- explicit StlStringBuffer(std::string data) : input_(std::move(data)) {
- if (!input_.empty()) {
- data_ = reinterpret_cast<const uint8_t*>(input_.c_str());
- size_ = static_cast<int64_t>(input_.size());
- capacity_ = size_;
- }
+std::shared_ptr<Buffer> Buffer::FromString(std::string data) {
+ if (data.empty()) {
+ return std::shared_ptr<Buffer>{new Buffer()};
}
- private:
- std::string input_;
-};
-
-std::shared_ptr<Buffer> Buffer::FromString(std::string data) {
- return std::make_shared<StlStringBuffer>(std::move(data));
+ auto size_in_bytes = static_cast<int64_t>(data.size());
+ return TakeOwnership(std::move(data), size_in_bytes);
}
Review Comment:
Buffer::FromString() now delegates to Buffer::TakeOwnership() with the
default get_data(), which returns a mutable pointer for std::string. This makes
FromString() produce a mutable buffer (and exposes std::string storage as
writable), contradicting the API contract and potentially allowing unsafe
mutation through MutableBuffer APIs.
##########
python/pyarrow/array.pxi:
##########
@@ -2269,6 +2274,51 @@ cdef class Array(_PandasConvertible):
return pyarrow_wrap_array(array)
+ @staticmethod
+ def from_dlpack(x, /, *, device=None, copy=None):
+ """
+ Construct an Array from an object implementing the DLPack protocol.
+
+ Parameters
+ ----------
+ x : object
+ The input object containing array data, following the DLPack
+ protocol (has a ``__dlpack__`` method).
+ device : tuple[enum.Enum, int], optional
+ Designates where the resulting Array should reside, in the
+ format returned by :meth:`Array.__dlpack_device__`. When None,
+ the output Array occupies the same device as the source.
+ Default: None.
+ copy : bool, optional
+ Controls duplication behavior. True mandates copying; False
+ prohibits copying and raises ``BufferError`` if unavoidable;
+ None duplicates only when necessary. Default: None.
+
+ Returns
+ -------
+ Array
+ An Array housing the data from the input object, potentially
+ as a copy or view.
+ """
+ version = (DLPACK_VERSION.major, DLPACK_VERSION.minor)
+ pycapsule = x.__dlpack__(max_version=version, dl_device=device,
copy=copy)
+ if not PyCapsule_CheckExact(pycapsule):
+ raise TypeError("DLPack producer did not return a PyCapsule")
+ cdef DLManagedTensorVersioned* ptr =
<DLManagedTensorVersioned*>PyCapsule_GetPointer(
+ pycapsule, "dltensor_versioned")
+ if ptr == NULL:
+ raise ValueError(
+ 'DLPack producer did not produce a "dltensor_versioned"
PyCapsule')
+ # Mark the capsule as consumed so its destructor does not also invoke
the deleter.
+ # ImportArrayVersionedFromDLPack will take ownership even if it errors
(calling
+ # the deleter in that case).
+ PyCapsule_SetName(pycapsule, "used_dltensor_versioned")
Review Comment:
PyCapsule_SetName() return value is ignored. If renaming the capsule fails,
proceeding to import risks double-free (capsule destructor + Import* deleter)
or leaving an unconsumed capsule state. Check the return code and raise before
calling ImportArrayVersionedFromDLPack (and apply the same pattern in
Tensor.from_dlpack).
##########
cpp/src/arrow/c/dlpack.h:
##########
@@ -105,4 +108,32 @@ Result<DLDevice> ExportDevice(const
std::shared_ptr<Array>& arr);
ARROW_EXPORT
Result<DLDevice> ExportDevice(const std::shared_ptr<Tensor>& t);
+/// \brief Import a DLPack tensor as an Arrow Array.
+///
+/// Same restrictions on data types as `ExportArrayVersioned`, only row-major
+/// tensors are supported. Takes ownership of the `DLManagedTensorVersioned` in
+/// an error-safe fashion.
+///
+/// \param[in] raw DLPack tensor
+/// \param[in] copy Whether to copy the data instead of sharing it with the
DLPack
+/// producer.
+/// \return An Arrow Array
+ARROW_EXPORT
+Result<std::shared_ptr<Array>> ImportArrayVersioned(DLManagedTensorVersioned*
raw,
+ bool copy);
+
+/// \brief Import a DLPack tensor as an Arrow Tensor.
+///
+/// Same restrictions on data types as `ExportTensorVersioned`.
+/// Takes ownership of the `DLManagedTensorVersioned` in an error-safe fashion.
+/// If the DLPack input is marked as readonly, this will produce an immutable
tensor.
+///
+/// \param[in] raw Arrow array
Review Comment:
ImportTensorVersioned() docstring incorrectly says `raw` is an "Arrow
array". The parameter is a DLPack tensor (DLManagedTensorVersioned*), which
matters for API consumers reading the header.
##########
python/pyarrow/array.pxi:
##########
@@ -4969,6 +5019,32 @@ cdef class FixedShapeTensorArray(ExtensionArray):
return self.to_tensor().to_numpy()
+ @staticmethod
+ def from_tensor(Tensor tensor not None):
+ """
Review Comment:
FixedShapeTensorArray.from_tensor() is a new public Python API but there
doesn't appear to be Python-level test coverage for it (existing tests cover
from_numpy_ndarray() but not from_tensor()). Adding a small test that
round-trips Tensor -> FixedShapeTensorArray -> Tensor (including
strided/permuted tensors) would help prevent regressions in shape/permutation
handling.
--
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]
