Copilot commented on code in PR #51122:
URL: https://github.com/apache/arrow/pull/51122#discussion_r3955455617
##########
cpp/src/arrow/buffer.h:
##########
@@ -146,14 +147,66 @@ class ARROW_EXPORT Buffer {
}
}
- /// \brief Construct an immutable buffer that takes ownership of the contents
+ /// \brief Default data accessor used by TakeOwnership.
+ struct DefaultGetData {
+ template <typename T>
+ auto* operator()(T& container) const {
+ return container.data();
+ }
+ };
+
+ /// \brief Construct an immutable buffer that takes ownership of a container.
Review Comment:
The docstring says "Construct an immutable buffer" but the function can
intentionally return a `MutableBuffer` depending on the owned container (and
`FromString` / `FromVector` now do). Update the wording to avoid contradicting
the behavior.
##########
python/pyarrow/array.pxi:
##########
@@ -2269,6 +2274,54 @@ 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.
+ Only 1 dimensional contiguous tensor are accepted as input.
+ For multi-dimensional tensor, first use `Tensor.from_dpack`, then
+ convert to an array using `FixedShapeTensorArray.from_tensor()`.
Review Comment:
Docstring has a typo and misleading guidance: it says `Tensor.from_dpack`
(typo) and uses ungrammatical wording. This is user-facing API documentation
for `Array.from_dlpack`.
This issue also appears on line 2315 of the same file.
##########
python/pyarrow/tensor.pxi:
##########
@@ -300,7 +306,52 @@ strides: {self.strides}"""
buffer.strides = <Py_ssize_t *>
cp.PyBytes_AsString(self._ssize_t_strides)
buffer.suboffsets = NULL
- def __dlpack__(self, stream=None, max_version=None, dl_device=None,
copy=None):
+ @staticmethod
+ def from_dlpack(x, /, *, device=None, copy=None):
+ """
+ Construct a Tensor 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 Tensor should reside, in the
+ format returned by :meth:`Tensor.__dlpack_device__`. When None,
+ the output Tensor 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
+ -------
+ Tensor
+ A Tensor 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.
+ # ImportTensorVersionedFromDLPack will take ownership even if it
errors (calling
+ # the deleter in that case).
+ PyCapsule_SetName(pycapsule, "used_dltensor_versioned")
+ with nogil:
Review Comment:
`PyCapsule_SetName` return value is ignored. If it fails, the capsule may
still call its destructor deleter while Arrow also takes ownership, risking
double-free. Check for failure and raise before importing.
--
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]
