[andishgar]

Rationale for this change

As mentioned here case 1.

What changes are included in this PR?

Handle negative zero in sparse tensor creation.

Are these changes tested?

Yes, I ran the relevant unit tests.

Are there any user-facing changes?

No.

This PR contains a "Critical Fix".
(If the changes fix either (a) a security vulnerability, (b) a bug that causes incorrect or invalid data to be produced, or (c) a bug that causes a crash—even when the API contract is upheld—please provide an explanation. If not, you can remove this.)

Reference: case 1

• GitHub Issue: [C++][Tensor] Incorrect logic for creating arrow::SparseCOOTensor from Tensor via arrow::SparseCOOTensor::Make (illegal memory access and incorrect values) #47520

[github-actions]

⚠️ GitHub issue #47520 has been automatically assigned in GitHub to PR creator.

[andishgar]

@rok, could you review this?

[rok]

I've done a high over pass and posted some questions. I want to do a deeper pass in more details later this week.

The templating approach seems more maintainable. We should make sure everything is tested.

[rok]

Thanks for the update @andishgar !
I read through the logic and things look good to me. It's especially nice that element size calculations are no longer done by us. Some minor comments, but overall I think this is practically ready to merge.
I would now ask @pitrou to do a pass, especially on the template part.

[rok]

I have no further comments. I'll add this to the release milestone and hopefully @pitrou has time to review it before release. (Many Arrow contributors are going to PyData Paris next week so I'm not sure how long things will take.)

[raulcd]

is this ready for review again? this is marked as blocker for the release along with 3 other pending issues

[andishgar]

@raulcd
I could finish in about 5 or 6 hours; however, there’s an issue — after the last review, I created the following code file, and it requires a more thorough investigation.

// Licensed to the Apache Software Foundation (ASF) under one
// or more contributor license agreements.  See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership.  The ASF licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License.  You may obtain a copy of the License at
//
//   http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied.  See the License for the
// specific language governing permissions and limitations
// under the License.

#pragma once

#include "arrow/sparse_tensor.h"
#include "arrow/status.h"
#include "arrow/type.h"
#include "arrow/visit_type_inline.h"

namespace arrow::util {

namespace detail {

struct ValueVisitor {
  template <typename ValueType, typename Function, typename... Args>
  enable_if_number<ValueType, Status> Visit(const ValueType& value_type,
                                            Function&& function, Args&&... args) {
    return function(value_type, std::forward<Args>(args)...);
  }

  template <typename... Args>
  Status Visit(const DataType& value_type, Args&&... args) {
    return Status::TypeError("Invalid value type: ", value_type.name(),
                             ". Expected a number.");
  }
};

struct IndexVisitor {
  template <typename IndexType, typename Function, typename... Args>
  enable_if_integer<IndexType, Status> Visit(const IndexType& index_type,
                                             Function&& function,
                                             const DataType& value_type, Args&&... args) {
    ValueVisitor visitor;
    return VisitTypeInline(value_type, &visitor, std::forward<Function>(function),
                           index_type, std::forward<Args>(args)...);
  }

  template <typename... Args>
  Status Visit(const DataType& index_type, Args&&...) {
    return Status::TypeError("Invalid index pointer type: ", index_type.name(),
                             ". Expected integer.");
  }
};

struct IndexPointerVisitor {
  template <typename IndexPointerType, typename Function>
  enable_if_integer<IndexPointerType, Status> Visit(
      const IndexPointerType& index_pointer_type, Function&& function,
      const DataType& index_type, const DataType& value_type) {
    IndexVisitor visitor;
    return VisitTypeInline(index_type, &visitor, std::forward<Function>(function),
                           value_type, index_pointer_type);
  }

  template <typename... Args>
  Status Visit(const DataType& index_pointer_type, Args&&...) {
    return Status::TypeError("Invalid index pointer type: ", index_pointer_type.name(),
                             ". Expected integer.");
  }
};
}  // namespace detail

template <typename Function>
inline Status VisitCSXType(const DataType& value_type, const DataType& index_type,
                           const DataType& indptr_type, Function&& function) {
  detail::IndexPointerVisitor visitor;
  return VisitTypeInline(indptr_type, &visitor, std::forward<Function>(function),
                         index_type, value_type);
}

template <typename Function>
inline Status VisitCOOTensorType(const DataType& value_type, const DataType& index_type,
                                 Function&& function) {
  detail::IndexVisitor visitor;
  return VisitTypeInline(index_type, &visitor, std::forward<Function>(function),
                         value_type);
}

}  // namespace arrow::util

[raulcd]

Thank you very much! There are still 3 other blockers for the release that are actively being worked on, they won't probably be ready until Monday. I am just trying to understand the current status in order to proceed with the release tasks. Thanks for your work on this issue!

[andishgar]

@rok @pitrou

With the Visitor interface in arrow/util/sparse_tensor_util.h, it’s possible to replace SparseTensorConverterMixin in arrow/tensor/converter_internal.h with a template. What do you think? Should I do that in this PR? (It should be relatively easy.)

[pitrou]

Thanks a lot for this update @andishgar . It looks generally good, but here are some more comments.

[andishgar]

@pitrou
One thing you might have overlooked during your code review is that the IndexPointer and Index can have different types. Therefore, VisitCSXType must support both types.

However, during SparseTensor creation from a Tensor via the Make method, the current interface only supports a single type for both the IndexPointer and the Index. It’s still possible, though, to create a SparseTensor via its constructor with different types for IndexPointer and Index.

Thus, it’s crucial to have a visit method that handles three types for sparse tensor validation. However, it’s not necessary to visit three types during the tensor-to-sparse-tensor conversion.

[pitrou]

Hmm, the problem is that this PR is leading to a combinatorial explosion of code generation that has very noticeable effects.

Here is library code size before this PR:

$ size -G /build/build-release/relwithdebinfo/lib*.so
      text       data        bss      total filename
   1231288     436476       1080    1668844 /build/build-release/relwithdebinfo/libarrow_acero.so
  11741720    2078723      46256   13866699 /build/build-release/relwithdebinfo/libarrow_compute.so
   1029085     613682       3496    1646263 /build/build-release/relwithdebinfo/libarrow_dataset.so
  11380712    3898809    2184321   17463842 /build/build-release/relwithdebinfo/libarrow.so
   1010359     500136       1992    1512487 /build/build-release/relwithdebinfo/libarrow_testing.so
   2676427    1510937       5536    4192900 /build/build-release/relwithdebinfo/libparquet.so

And here it is after this PR:

$ size -G /build/build-release/relwithdebinfo/lib*.so
      text       data        bss      total filename
   1231288     436476       1080    1668844 /build/build-release/relwithdebinfo/libarrow_acero.so
  11741720    2078723      46256   13866699 /build/build-release/relwithdebinfo/libarrow_compute.so
   1029085     613682       3496    1646263 /build/build-release/relwithdebinfo/libarrow_dataset.so
  15511528    4319214    2183753   22014495 /build/build-release/relwithdebinfo/libarrow.so
   1010359     500136       1992    1512487 /build/build-release/relwithdebinfo/libarrow_testing.so
   2676427    1510937       5536    4192900 /build/build-release/relwithdebinfo/libparquet.so

We cannot afford to add 4MB of code (a 37% increase for libarrow.so) just to make sparse tensors more correct. So we need to find an approach that generates much less code.

[andishgar]

@pitrou
What size increase is acceptable? I was able to limit the scope of the template and reduce the overhead on my PC from 5 megabytes to 800 kilobytes. Is this acceptable, or should I reduce it further?

[pitrou]

Given this is a little-used part of the code, I would ideally like to reduce it further, if at all possible.

[andishgar]

Yes, it is possible. I could replace all pointer arithmetic related to Index and IndexPointer with SparseTensorConverterMixin in arrow/tensor/converter_internal.h. This could potentially reduce code generation—by up to eight times.

[andishgar]

@pitrou
I noticed that in my latest commit, the binary size is 17 KB smaller than in the main branch :-)

[andishgar]

@rok
I removed all the code related to the conversion from a column-major Tensor to a SparseCOOTensor and instead reused the logic from StridedTensor for this conversion. My reasons are:

1. It uses std::vector for large allocations instead of arrow::AllocateBuffer.
2. It doesn’t use the result of sorting, which wastes a lot of CPU cycles.

I know that the current approach may reduce the benefits of CPU cache usage. However, since the existing logic is problematic, I think it’s better to use the StridedTensor logic as a temporary solution until a better approach is implemented.

Given all this, are you okay with such a change?

[andishgar]

@pitrou, according to this PR, should this issue be closed?

[pitrou]

@andishgar Well, I have no idea about the future of the SparseTensor classes. The PR you link to is about deprecating Tensor/SparseTensor in the IPC format, but not necessarily in the Arrow C++ codebase.

(for example, C++ Tensor can map to the fixed shape tensor extension type; we have no such mapping for C++ SparseTensor, for now)