Uh oh!
There was an error while loading. Please reload this page.
GH-38868: [C++][Python] Add Array::ToTensor and fixed size list support - #50929
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new public Array::ToTensor API (C++ and Python) to enable exporting multidimensional array-like data as Tensor, and updates DLPack export paths and tests to use to_tensor() for multidimensional support (notably nested FixedSizeListArray and FixedShapeTensorArray).
Changes:
- Add virtual
Array::ToTensorplus concrete implementations for 1D numeric arrays and (nested) fixed-size list arrays; routeFixedShapeTensorArray::ToTensorthrough the base virtual. - Refactor tensor stride utilities (row-major stride computation) and simplify DLPack device handling; update DLPack type errors to suggest Tensor conversion.
- Add/extend C++ and Python test coverage for
to_tensor().__dlpack__()on multidimensional inputs.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| python/pyarrow/tests/test_dlpack.py | Adds multidimensional array-to-tensor DLPack export tests via arr.to_tensor() |
| python/pyarrow/includes/libarrow.pxd | Exposes Array::ToTensor() at the Cython API layer |
| python/pyarrow/array.pxi | Adds Array.to_tensor() Python API and routes FixedShapeTensorArray.to_tensor() through it |
| cpp/src/arrow/tensor.h | Updates stride utilities API and adds std::span overload for row-major strides |
| cpp/src/arrow/tensor.cc | Refactors row-major stride computation implementation |
| cpp/src/arrow/extension/fixed_shape_tensor.h | Makes FixedShapeTensorArray::ToTensor() override the new virtual |
| cpp/src/arrow/extension/fixed_shape_tensor.cc | Updates ToTensor() signature to match override |
| cpp/src/arrow/c/dlpack.cc | Refactors DLPack export (device factoring, type checks, array offset/length handling) and updates type errors |
| cpp/src/arrow/c/dlpack_test.cc | Updates DLPack tests to validate shape/strides and revised ExportDevice behavior |
| cpp/src/arrow/array/array_test.cc | Adds C++ unit tests for Array::ToTensor() on primitive arrays |
| cpp/src/arrow/array/array_primitive.h | Implements NumericArray::ToTensor() for 1D numeric arrays |
| cpp/src/arrow/array/array_nested.h | Declares FixedSizeListArray::ToTensor() API |
| cpp/src/arrow/array/array_nested.cc | Implements FixedSizeListArray::ToTensor() with nested fixed-size list support |
| cpp/src/arrow/array/array_list_test.cc | Adds tests for FixedSizeListArray::ToTensor() including nesting, slicing, and null handling |
| cpp/src/arrow/array/array_base.h | Declares new virtual Array::ToTensor() API |
| cpp/src/arrow/array/array_base.cc | Provides default Array::ToTensor() NotImplemented behavior |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
python/pyarrow/tests/test_dlpack.py:169
- The numpy version guard checks
< 1.24.0, but the skip message says "No dlpack support ... older than 1.22.0". This is confusing when diagnosing test skips; update the message to reflect the actual minimum version (and optionally mention why 1.24 is required).
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")
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (3)
python/pyarrow/tests/test_dlpack.py:169
- The skip condition is
numpy < 1.24.0, but the message says "older than 1.22.0". This is misleading when diagnosing CI skips; align the message with the actual version gate (or explain both requirements explicitly).
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")
cpp/src/arrow/array/array_test.cc:1234
- Two of the EXPECT_EQ assertions are no-ops (they compare
shape/stridesto literals that exactly match those variables), so this test isn't actually verifying the tensor shape/strides beyond the later checks. Removing them makes the intent clearer and avoids false confidence in coverage.
EXPECT_EQ(int32(), tensor->type());
EXPECT_EQ(shape, std::vector<int64_t>{5});
EXPECT_EQ(strides, std::vector<int64_t>{sizeof(int32_t)});
EXPECT_EQ(shape, tensor->shape());
EXPECT_EQ(strides, tensor->strides());
cpp/src/arrow/extension/fixed_shape_tensor.h:48
- Docstring grammar: "where this array null entries" is missing a verb. This is a public header comment, so it's worth fixing for clarity.
/// Nulls are ignored, leaving the output tensor with unspecified values where this
/// array null entries.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
cpp/src/arrow/array/array_base.h:257
- Public API comment has a couple of grammatical issues ("Example include" / "where this array null entries"), which can be confusing in generated docs.
/// Example include NumericArray, FixedShapeTensorArray, nested FixedSizeListArray.
/// Nulls are ignored, leaving the output tensor with unspecified values where this
/// array null entries.
cpp/src/arrow/array/array_nested.h:653
- Doc comment contains grammatical issues ("number of element", "fixed sized list", "where this array null entries"). Since this is a public override, it will show up in generated docs.
/// The output tensor has a row major layout with the number of element as the first
/// dimension and the fixed sized list as the remaining one (possibly nested).
/// Nulls are ignored, leaving the output tensor with unspecified values where this
/// array null entries.
python/pyarrow/tests/test_dlpack.py:169
- The skip condition is
numpy < 1.24.0, but the message says "No dlpack support ... older than 1.22.0". This is misleading for numpy 1.22/1.23 where dlpack exists but the test still needs 1.24 due tostrict=True.
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")
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
python/pyarrow/array.pxi:1856
- Docstring says allow_nulls defaults to True, but the function signature defaults to False. This mismatch will confuse users and docs generation.
allow_nulls: bool, default `True`
cpp/src/arrow/array/array_nested.cc:1035
- If the leaf values buffer is null for an empty FixedSizeListArray (possible for length==0 ArrayData), this passes a null data buffer to Tensor::Make, which fails validation. Consider creating an explicit 0-byte Buffer when the computed tensor size is 0.
std::shared_ptr<Buffer> buffer = nullptr;
if (const auto& buf = data->buffers[1]; buf != NULLPTR) {
const int64_t byte_width = type->byte_width();
// Buffer guarantees this fits into an int64_t.
const int64_t byte_offset = offset * byte_width;
cpp/src/arrow/array/array_primitive.h:152
- If a (valid) empty NumericArray has a null values buffer (buffers[1] == nullptr), this method passes a null data buffer into Tensor::Make, which fails validation even though the tensor has zero elements. Consider materializing an explicit 0-byte Buffer in that case.
std::shared_ptr<Buffer> buffer;
if (data_->buffers[1] != NULLPTR) {
// Array guarantees this will not overflow.
const int64_t byte_offset = data_->offset * byte_width;
const int64_t byte_length = length() * byte_width;
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
AntoinePrv
commented
Sep 1, 2026
@pitrou I changed |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
python/pyarrow/array.pxi:1856
allow_nullsis documented as defaulting to True, but the signature defaults to False. This makes the docstring misleading and contradicts the behavior tested elsewhere (nulls rejected unless explicitly allowed).
Parameters
----------
allow_nulls: bool, default `True`
When true, nulls are ignored, leaving the output tensor with
python/pyarrow/array.pxi:4995
- This override of
FixedShapeTensorArray.to_tensor()shadowsArray.to_tensor(allow_nulls=...)but doesn't accept anallow_nullsargument. As a result,arr.to_tensor(allow_nulls=True)will raiseTypeErrorfor fixed-shape tensor arrays, breaking the new null-handling API and the added tests.
"""
return Array.to_tensor(self)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
python/pyarrow/array.pxi:1856
- The docstring says
allow_nullsdefaults toTrue, but the Python signature defaults toFalse(and the C++ default is alsofalse). This is user-facing API documentation, so it should match the actual default behavior.
allow_nulls: bool, default `True`
python/pyarrow/array.pxi:4995
- This delegation uses
Array.to_tensor(self)without exposing the newallow_nullsparameter. ForFixedShapeTensorArrayinstances,arr.to_tensor(allow_nulls=True)will raise a PythonTypeErrorbecause this override’s signature isto_tensor(self)only.
return Array.to_tensor(self)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cpp/src/arrow/c/dlpack.cc:54
- Typo/wording: "multi dimensional" should be hyphenated as "multi-dimensional" in this user-facing error message (and update test expectations accordingly).
return Status::TypeError(
"DataType is not compatible with DLPack spec: ", type.ToString(),
", try converting to a Tensor for multi dimensional data support");
}
cpp/src/arrow/array/array_nested.cc:1023
offset = offset * list_size + ...andlength = length * list_sizecan trigger signed overflow (UB) on large arrays beforeSliceBufferSafehas a chance to validate bounds. Use overflow-checked arithmetic to keep this safe even on malformed/unvalidated inputs.
// Overflow cannot happen on a valid array (its data needs to fit in memory,
// therefore be smaller than INT64_MAX)
offset = offset * fsl->list_size() + data->offset;
length = length * fsl->list_size();
shape.push_back(fsl->list_size());
pitrou
left a comment
There was a problem hiding this comment.
A bunch of nits and minor suggestions, but LGTM in general!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| # A Tensor sharing an Array buffer is immutable, so it can only be exported | ||
| # through the versioned DLPack protocol. | ||
| assert not tensor.is_mutable | ||
| result = np.from_dlpack(DLPackForwarder(tensor, max_version=(1, 0))) |
There was a problem hiding this comment.
Is it possible to also call np.from_dlpack(arr) or is that not possible yet?
There was a problem hiding this comment.
It seems latest numpy does not support max_version argument.
There was a problem hiding this comment.
I was thinking more about this:
>>>a=pa.FixedShapeTensorArray.from_numpy_ndarray(np_arr)
>>>a.__dlpack__()
Traceback (mostrecentcalllast):
CellIn[10], line1a.__dlpack__()
Filepyarrow/array.pxi:2331inpyarrow.lib.Array.__dlpack__legacy_tensor=GetResultValue(ExportArrayToDLPack(self.sp_array))
Filepyarrow/error.pxi:155inpyarrow.lib.pyarrow_internal_check_statusreturncheck_status(status)
Filepyarrow/error.pxi:92inpyarrow.lib.check_statusraiseconvert_status(status)
ArrowTypeError: DataTypeisnotcompatiblewithDLPackspec: extension<arrow.fixed_shape_tensor[value_type=int32, shape=[2,2], permutation=[0,1]]>, tryconvertingtoaTensorformultidimensionaldatasupport/home/antoine/arrow/dev/cpp/src/arrow/c/dlpack.cc:131GetDLDataType(type)It would be nice to make it work at some point (perhaps not in this PR?).
11adc24 to
3fb604fCompareAntoinePrv
commented
Sep 3, 2026
@pitrou this is ready |
There was a problem hiding this comment.
🔵 Needs a closer look
There are minor but user-facing error-message spelling inconsistencies (and corresponding test expectations) that should be corrected before merging.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
cpp/src/arrow/c/dlpack_test.cc:174
- This expected message also concatenates "multi" + " dimensional"; if the production error message is corrected to "multidimensional", update this string literal accordingly so the assertion continues to match.
This issue also appears on line 184 of the same file.
cpp/src/arrow/c/dlpack.cc:53
- The error message says "multi dimensional"; this should be "multidimensional" (single word) to avoid awkward phrasing in a user-facing TypeError.
return Status::TypeError(
"DataType is not compatible with DLPack spec: ", type.ToString(),
", try converting to a Tensor for multi dimensional data support");
cpp/src/arrow/c/dlpack_test.cc:189
- Same as above: the expected message currently builds "multi" + " dimensional". If the production error message is normalized to "multidimensional", this assertion should be updated to match.
ASSERT_RAISES_WITH_MESSAGE(TypeError,
"Type error: DataType is not compatible with DLPack spec: " +
array_string->type()->ToString() +
", try converting to a Tensor for multi"
" dimensional data support",
TypeParam::Export(array_string));
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
Rationale for this change
Enable multidimensional DLPack support for Array via
to_tensor.What changes are included in this PR?
Array::ToTensorNumericArray::ToTensorfor 1D arraysFixedSizeListArray::ToTensorfor multidimensional arraysarr.to_tensor().__dlpack__()Note: Nulls are explicitly supported in
to_tensoras unspecified data. This was the current behaviour.Are these changes tested?
Yes
Are there any user-facing changes?
New public Array function.