Copilot commented on code in PR #50929:
URL: https://github.com/apache/arrow/pull/50929#discussion_r3822524334
##########
cpp/src/arrow/c/dlpack.cc:
##########
@@ -215,18 +208,10 @@ Result<DT*> ExportTensorImpl(const
std::shared_ptr<Tensor>& t, bool copy) {
const auto& type = *t->type();
ARROW_ASSIGN_OR_RAISE(auto dtype, GetDLDataType(type));
- // Compute strides
- std::vector<int64_t> strides = {};
- strides.reserve(t->ndim());
- const auto byte_width = type.byte_width();
- for (auto i : t->strides()) {
- strides.emplace_back(i / byte_width);
- }
-
auto params = ExportBufferParams<std::vector<int64_t>>{
- .size = t->size(),
+ .buffer_size = t->size(),
Review Comment:
ExportTensorImpl sets ExportBufferParams::buffer_size to t->size() (element
count), but the struct documents/uses buffer_size as a byte count (e.g.
ExportArrayImpl uses it as CopySlice nbytes). This inconsistency can lead to
incorrect behavior if buffer_size is later used beyond the empty-check.
##########
cpp/src/arrow/c/dlpack_test.cc:
##########
@@ -182,7 +191,11 @@ TYPED_TEST(TestExportArray, TestErrors) {
const std::shared_ptr<Array> array_boolean = ArrayFromJSON(boolean(),
"[true, false]");
ASSERT_RAISES_WITH_MESSAGE(
TypeError, "Type error: Bit-packed boolean data type not supported by
DLPack.",
- arrow::dlpack::ExportDevice(array_boolean));
+ TypeParam::Export(array_boolean));
Review Comment:
The expected error strings for unsupported types (null/utf8) no longer match
the updated TypeError message from GetDLDataType (which now appends a Tensor
conversion hint), so this test will fail.
##########
cpp/src/arrow/array/array_primitive.h:
##########
@@ -128,6 +130,19 @@ class NumericArray : public PrimitiveArray {
IteratorType end() const { return IteratorType(*this, length()); }
+ /// \brief Return a one dimensional Tensor.
+ Result<std::shared_ptr<Tensor>> ToTensor() const override {
+ // Could be non-templated
+ const int64_t byte_width = type()->byte_width();
+ std::shared_ptr<Buffer> buffer;
+ if (data_->buffers[1] != NULLPTR) {
+ const auto boffset = data_->offset * byte_width;
+ const auto blength = length() * byte_width;
+ ARROW_ASSIGN_OR_RAISE(buffer, SliceBufferSafe(data_->buffers[1],
boffset, blength));
+ }
+ return Tensor::Make(type(), std::move(buffer), {length()});
Review Comment:
If the values buffer is absent (data_->buffers[1] == NULLPTR), buffer
remains null and Tensor::Make will fail with "Null data is supplied" even for a
valid empty NumericArray. Consider providing a 0-length Buffer when no values
buffer exists.
##########
cpp/src/arrow/array/array_nested.cc:
##########
@@ -1001,6 +1001,40 @@ Result<std::shared_ptr<Array>>
FixedSizeListArray::Flatten(
return FlattenListArray(*this, memory_pool);
}
+Result<std::shared_ptr<Tensor>> FixedSizeListArray::ToTensor() const {
+ const auto* data = this->data().get();
+ auto type = this->type();
+ int64_t offset = data->offset;
+ int64_t length = data->length;
+ std::vector<int64_t> shape{length};
+
+ // Iterate over nested fixed length container types.
+ // Each nested container increase the tensor dimension.
+ while (type->id() == Type::FIXED_SIZE_LIST) {
+ const auto* fsl = internal::checked_cast<const
FixedSizeListType*>(type.get());
+ type = fsl->value_type();
+ data = data->child_data.front().get();
+
+ offset = offset * fsl->list_size() + data->offset;
+ length *= fsl->list_size();
+ shape.push_back(fsl->list_size());
+ }
+
+ // Only checking byte_width and leaving Tensor::Make error on unsupported
types.
+ if (!is_fixed_width(*type)) {
+ return Status::NotImplemented("Expected a fixed width leaf type, got ",
type->name());
+ }
+
+ std::shared_ptr<Buffer> buffer = nullptr;
+ if (const auto& buf = data->buffers[1]; buf != NULLPTR) {
+ const int64_t byte_width = type->byte_width();
+ ARROW_ASSIGN_OR_RAISE(buffer,
+ SliceBufferSafe(buf, offset * byte_width, length *
byte_width));
+ }
+
+ return Tensor::Make(std::move(type), std::move(buffer), std::move(shape));
Review Comment:
If the leaf values buffer is absent (data->buffers[1] == NULLPTR), buffer
stays null and Tensor::Make will fail with "Null data is supplied" even for a
valid empty FixedSizeListArray. Consider providing a 0-length Buffer when no
values buffer exists.
##########
python/pyarrow/tests/test_dlpack.py:
##########
@@ -145,6 +145,32 @@ def test_tensor_dlpack(np_type):
check_dlpack_export(t, expected)
+def multidim_arrays():
+ np_arr = np.arange(12, dtype=np.int32).reshape(3, 2, 2)
+ values = pa.array(np_arr.ravel(), type=pa.int32())
+ nested_list = pa.FixedSizeListArray.from_arrays(
+ pa.FixedSizeListArray.from_arrays(values, 2), 2)
+ return [
+ pytest.param(nested_list, np_arr, id="nested_fixed_size_list"),
+ pytest.param(
+ pa.FixedShapeTensorArray.from_numpy_ndarray(np_arr),
+ np_arr,
+ id="fixed_shape_tensor",
+ ),
+ ]
+
+
+@check_bytes_allocated
[email protected](('arr', 'expected'), multidim_arrays())
+def test_array_to_tensor_dlpack(arr, expected):
+ if Version(np.__version__) < Version("1.24.0"):
+ pytest.skip("No dlpack support in numpy versions older than 1.22.0, "
+ "strict keyword in assert_array_equal added in numpy
version "
+ "1.24.0")
Review Comment:
The skip message says "No dlpack support ... older than 1.22.0" but the
condition is numpy < 1.24.0. Update the message to reflect the actual minimum
required version.
--
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]
