Copilot commented on code in PR #51122:
URL: https://github.com/apache/arrow/pull/51122#discussion_r3959674334
##########
cpp/src/arrow/c/dlpack.cc:
##########
@@ -248,4 +261,234 @@ Result<DLDevice> ExportDevice(const
std::shared_ptr<Tensor>& t) {
return ExportDeviceImpl(t);
}
+/***************
+ * Consumers *
+ ***************/
+
+namespace {
+
+class CppDLTensor {
+ public:
+ using value_type = DLManagedTensorVersioned;
+ using pointer_type = value_type*;
+
+ static Result<CppDLTensor> TakeOwnership(pointer_type ptr) {
+ if (ARROW_PREDICT_FALSE(ptr == nullptr)) {
+ return Status::Invalid("Received null pointer.");
+ }
+ // Create the wrapper before checking the version as the spec mandates
that the
+ // deleter MUST be called on version major mismatch.
+ auto out = CppDLTensor(ptr);
+ if (ARROW_PREDICT_FALSE(out.ptr_->version.major != kVersion.major)) {
+ return Status::Invalid("Unsupported DLPack major version ",
out.ptr_->version.major,
+ ", expected ", kVersion.major);
+ }
+ if (ARROW_PREDICT_FALSE(out.tensor().ndim < 0)) {
+ return Status::Invalid("Invalid DLPack tensor: ndim must be >= 0");
+ }
+ if (ARROW_PREDICT_FALSE(out.tensor().ndim != 0 && out.tensor().shape ==
nullptr)) {
+ return Status::Invalid(
+ "Invalid DLPack tensor: shape must be non-null when ndim != 0");
+ }
+ if (ARROW_PREDICT_FALSE(out.tensor().ndim != 0 && out.tensor().strides ==
nullptr)) {
+ return Status::Invalid(
+ "Invalid DLPack tensor: strides must be non-null when ndim != 0");
+ }
Review Comment:
`CppDLTensor::TakeOwnership` rejects tensors with `strides == nullptr` when
`ndim != 0`, but `arrow/c/dlpack_abi.h` explicitly notes that (before DLPack
v1.2) NULL strides are allowed to indicate contiguous data. Since the importer
only checks `version.major`, this will reject valid v1.0/v1.1 producers (and
would also make later `dl.strides().front()` accesses unsafe if the check is
relaxed). Consider accepting NULL strides when `version.minor < 2` and treating
them as compact row-major strides computed from `shape` (and still rejecting
NULL strides for v1.2+).
##########
cpp/src/arrow/c/dlpack_test.cc:
##########
@@ -329,4 +329,361 @@ TYPED_TEST(TestExportTensor, TestTensorStrided) {
f_dlpack_strides);
}
+/***************
+ * Consumers *
+ ***************/
+
+/// A DLPack tensor as a foreign library would produce it.
+struct ForeignTensor {
+ DLDataType dtype = {.code = kDLFloat, .bits = 32, .lanes = 1};
+ std::vector<int64_t> shape = {};
+ /// In number of elements, as mandated by DLPack.
+ std::vector<int64_t> strides = {};
+ std::vector<uint8_t> data = {};
+ DLDevice device = {.device_type = kDLCPU, .device_id = 0};
+ uint64_t byte_offset = 0;
+ uint64_t flags = 0;
+ /// Incremented when the consumer releases the tensor.
+ std::shared_ptr<int> deleted = std::make_shared<int>(0);
+
+ DLManagedTensorVersioned managed = {};
+};
+
+template <typename T>
+std::vector<uint8_t> ToBytes(const std::vector<T>& values) {
+ std::vector<uint8_t> bytes(values.size() * sizeof(T));
+ std::memcpy(bytes.data(), values.data(), bytes.size());
+ return bytes;
+}
+
+/// Hand out a DLPack tensor owning ``foreign``, releasing it through its
deleter.
+DLManagedTensorVersioned* Produce(ForeignTensor foreign) {
+ auto owned = std::make_unique<ForeignTensor>(std::move(foreign));
+ owned->managed = {
+ .version = {.major = DLPACK_MAJOR_VERSION, .minor =
DLPACK_MINOR_VERSION},
+ .manager_ctx = owned.get(),
Review Comment:
The new consumer tests don't cover the pre-DLPack-v1.2 case where
`dl_tensor.strides` may be NULL to indicate contiguous data (documented in
`arrow/c/dlpack_abi.h`). Adding a regression test that sets
`managed->version.minor = 1` and `managed->dl_tensor.strides = nullptr` for a
compact row-major tensor would help ensure `Import{Array,Tensor}Versioned`
stays compatible with older producers.
--
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]
