AlenkaF commented on code in PR #50929:
URL: https://github.com/apache/arrow/pull/50929#discussion_r3863968114
##########
cpp/src/arrow/tensor.cc:
##########
@@ -45,34 +45,36 @@ using internal::checked_cast;
namespace internal {
-Status ComputeRowMajorStrides(const FixedWidthType& type,
- const std::vector<int64_t>& shape,
- std::vector<int64_t>* strides) {
- const int byte_width = type.byte_width();
- const size_t ndim = shape.size();
-
- int64_t remaining = 0;
- if (!shape.empty() && shape.front() > 0) {
- remaining = byte_width;
- for (size_t i = 1; i < ndim; ++i) {
- if (internal::MultiplyWithOverflow(remaining, shape[i], &remaining)) {
- return Status::Invalid(
- "Row-major strides computed from shape would not fit in 64-bit
integer");
- }
- }
+Status ComputeRowMajorStrides(std::span<const int64_t> shape, int64_t
elem_size,
+ std::span<int64_t> strides) {
+ if (strides.size() != shape.size()) {
+ return Status::Invalid("strides must have the same length as shape");
}
- if (remaining == 0) {
- strides->assign(shape.size(), byte_width);
+ // An empty dimension makes the whole tensor empty, so any stride is as good.
Review Comment:
The changes in tensor.h/.cc were not clear to me so I helped myself with
Claude to better understand. Is the reason behind the change (order and code
change) meant to catch an empty dimension up front?
##########
cpp/src/arrow/tensor.h:
##########
@@ -56,10 +57,17 @@ constexpr bool is_tensor_supported(Type::type type_id) {
namespace internal {
ARROW_EXPORT
-Status ComputeRowMajorStrides(const FixedWidthType& type,
- const std::vector<int64_t>& shape,
+Status ComputeRowMajorStrides(const FixedWidthType& type, std::span<const
int64_t> shape,
std::vector<int64_t>* strides);
+/// Compute the row-major strides of a tensor with the given shape.
+///
+/// Pass `elem_size=1` to get the strides in number of elements, or the
element size in
+/// bytes to get them in bytes. On error, the contents of `strides` are
unspecified.
+ARROW_EXPORT
+Status ComputeRowMajorStrides(std::span<const int64_t> shape, int64_t
elem_size,
Review Comment:
Is there a specific reason why the API changed (new signature added, the old
becoming a thin wrapper)? Is this meant to serve a purpose for DLPack or is
this a general improvement? cc @rok
##########
cpp/src/arrow/array/array_list_test.cc:
##########
@@ -1821,4 +1822,106 @@ TEST_F(TestFixedSizeListArray, FlattenRecursively) {
*ArrayFromJSON(value_type_, "[0, 1, null, 3, 7, null, 2,
5]"));
}
+namespace {
+
+/// The flat values a tensor views: the innermost values of the nested fixed
size
+/// lists, windowed to what ``array`` covers.
Review Comment:
This comment confused me. Maybe something simpler, like "flat values" or
"innermost values" would already be close to enough?
##########
cpp/src/arrow/extension/fixed_shape_tensor.h:
##########
@@ -43,7 +43,10 @@ class ARROW_EXPORT FixedShapeTensorArray : public
ExtensionArray {
/// dimension as length equal to the FixedShapeTensorArray's length and the
remaining
/// dimensions as the FixedShapeTensorType's shape. Shape and dim_names will
be
/// permuted according to permutation stored in the FixedShapeTensorType
metadata.
- const Result<std::shared_ptr<Tensor>> ToTensor() const;
+ ///
+ /// Nulls are ignored, leaving the output tensor with unspecified values
where this
+ /// array has null entries.
Review Comment:
👍
--
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]
