Copilot commented on code in PR #51122:
URL: https://github.com/apache/arrow/pull/51122#discussion_r3927039635
##########
cpp/src/arrow/tensor.cc:
##########
@@ -109,13 +109,40 @@ Status ComputeColumnMajorStrides(const FixedWidthType&
type,
return Status::OK();
}
-} // namespace internal
+Result<int64_t> ComputeTensorSize(std::span<const int64_t> shape,
+ std::span<const int64_t> strides, int64_t
elem_size) {
+ // Check the largest offset can be computed without overflow
+ const size_t ndim = shape.size();
+ int64_t largest_offset = elem_size;
+ for (size_t i = 0; i < ndim; ++i) {
+ if (shape[i] == 0) continue;
+ if (strides[i] < 0) {
+ // TODO(mrkn): Support negative strides for sharing views
+ return Status::Invalid("negative strides not supported");
+ }
-namespace {
+ int64_t dim_offset = 0;
+ if (!internal::MultiplyWithOverflow(shape[i] - 1, strides[i],
&dim_offset)) {
+ if (!internal::AddWithOverflow(largest_offset, dim_offset,
&largest_offset)) {
+ continue;
+ }
+ }
-inline bool IsTensorStridesRowMajor(const std::shared_ptr<DataType>& type,
- const std::vector<int64_t>& shape,
- const std::vector<int64_t>& strides) {
+ return Status::Invalid(
+ "offsets computed from shape and strides would not fit in 64-bit
integer");
+ }
+
+ // A dimension with no element means empty for which the preceding does not
apply.
+ if (std::find(shape.begin(), shape.end(), 0) != shape.end()) {
+ return 0;
+ }
+ return largest_offset;
+}
+
+namespace {
+bool IsTensorStridesRowMajor(const std::shared_ptr<DataType>& type,
+ const std::vector<int64_t>& shape,
+ const std::vector<int64_t>& strides) {
std::vector<int64_t> c_strides;
Review Comment:
`IsTensorStridesRowMajor` / `IsTensorStridesColumnMajor` are defined inside
an anonymous namespace nested under `arrow::internal` (starting at `namespace
{`), but later calls use `internal::IsTensorStridesRowMajor/ColumnMajor` and
`IsTensorStridesContiguous()` (in `arrow::internal`) calls them unqualified.
Those lookups won’t find the nested-anonymous definitions, which should cause
compile errors.
##########
cpp/src/arrow/c/dlpack.h:
##########
@@ -105,4 +108,33 @@ 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`, and only
row-major
+/// tensors are supported. Dimensions beyond the first are imported as nested
+/// fixed size lists.
+/// Takes ownership of the `DLManagedTensorVersioned` though it may point to
shared data.
Review Comment:
The ImportArrayVersioned documentation claims multidimensional tensors are
imported as nested fixed-size lists, but the implementation rejects `ndim != 1`
(see `ImportArrayVersioned` in `dlpack.cc`). The header comment should match
the actual supported behavior.
This issue also appears on line 132 of the same file.
##########
python/pyarrow/tensor.pxi:
##########
@@ -300,7 +302,50 @@ 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):
+ def from_dlpack(x, /, *, device=None, copy=None):
Review Comment:
`Tensor.from_dlpack` is a factory method but is not marked as
`@staticmethod` (unlike `Tensor.from_numpy` and other `from_*` constructors).
This makes binding/inspection inconsistent and allows accidental instance
binding.
##########
python/pyarrow/array.pxi:
##########
@@ -2269,6 +2274,48 @@ cdef class Array(_PandasConvertible):
return pyarrow_wrap_array(array)
+ def from_dlpack(x, /, *, device=None, copy=None):
Review Comment:
`Array.from_dlpack` is a factory method but is not marked as `@staticmethod`
like other `Array.from_*` constructors in this file. Without `@staticmethod`,
it can also be (mis)bound as an instance method and the signature shown in
docs/introspection can be confusing.
##########
python/pyarrow/tensor.pxi:
##########
@@ -300,7 +302,50 @@ 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):
+ 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) or the array API's
+ ``__array_namespace__`` protocol.
Review Comment:
The `Tensor.from_dlpack` docstring claims inputs may follow the array API
`__array_namespace__` protocol, but the implementation only calls
`x.__dlpack__(...)`. Either add support for the array API path or remove this
from the docs to avoid misleading users.
--
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]
