GH-47520: [C++][Tensor] Correct sparse tensor creation from dense tensor with negative zero

[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.)

[rok]

@pitrou could you per-chance review this before release? (we've already done multiple iterations)

[pitrou]

Ok, I tried to read this PR but this is making many changes that do not seem related to the task of fixing a simple bug. Can we have a more minimal set of changes?

[andishgar]

@pitrou Thanks for the feedback!

Ok, I tried to read this PR but this is making many changes that do not seem related to the task of fixing a simple bug.
Can we have a more minimal set of changes?

The issue is that the current zero-checking logic is incorrect across all sparse tensor creation paths, and that can lead to memory issues when tensors are created from negative zero values. So the fix touches several places that share this logic.

Do you think it would still make sense to split it further? (This PR is already a breakdown of a larger patch

[pitrou]

Ok, I'll take another look here.

[andishgar]

@pitrou @rok
I’ve identified another issue while applying this change.It appears that NaN values are not handled correctly.
To clarify, the patches below demonstrate that NaN values are not properly managed during sparse tensor creation.
Would you prefer that I address this within the current pull request, or should I open a separate issue to track it?

diff --git a/cpp/src/arrow/sparse_tensor_test.cc b/cpp/src/arrow/sparse_tensor_test.cc
index c9c28a1..6aa5835 100644
--- a/cpp/src/arrow/sparse_tensor_test.cc
+++ b/cpp/src/arrow/sparse_tensor_test.cc
@@ -499,6 +499,8 @@ TYPED_TEST_P(TestFloatingSparseCOOTensorEquality, TestEquality) {
   ASSERT_OK_AND_ASSIGN(st4, SparseCOOTensor::Make(*this->tensor2_));
   EXPECT_FALSE(st4->Equals(*st4));                                  // same object
   EXPECT_TRUE(st4->Equals(*st4, EqualOptions().nans_equal(true)));  // same object
+  ASSERT_OK_AND_ASSIGN(auto my_tensor,st4->ToTensor());
+  ASSERT_TRUE(my_tensor->Equals(*this->tensor2_));
 
   std::vector<c_value_type> values5 = this->values2_;
   std::shared_ptr<SparseCOOTensor> st5;
@@ -955,6 +957,8 @@ TYPED_TEST_P(TestFloatingSparseCSRMatrixEquality, TestEquality) {
   ASSERT_OK_AND_ASSIGN(st4, SparseCSRMatrix::Make(*this->tensor2_));
   EXPECT_FALSE(st4->Equals(*st4));                                  // same object
   EXPECT_TRUE(st4->Equals(*st4, EqualOptions().nans_equal(true)));  // same object
+  ASSERT_OK_AND_ASSIGN(auto my_tensor,st4->ToTensor());
+  ASSERT_TRUE(my_tensor->Equals(*this->tensor2_));
 
   std::vector<c_value_type> values5 = this->values2_;
   std::shared_ptr<SparseCSRMatrix> st5;
@@ -1290,6 +1294,8 @@ TYPED_TEST_P(TestFloatingSparseCSCMatrixEquality, TestEquality) {
   ASSERT_OK_AND_ASSIGN(st4, SparseCSCMatrix::Make(*this->tensor2_));
   EXPECT_FALSE(st4->Equals(*st4));                                  // same object
   EXPECT_TRUE(st4->Equals(*st4, EqualOptions().nans_equal(true)));  // same object
+  ASSERT_OK_AND_ASSIGN(auto my_tensor,st4->ToTensor());
+  ASSERT_TRUE(my_tensor->Equals(*this->tensor2_));
 
   std::vector<c_value_type> values5 = this->values2_;
   std::shared_ptr<SparseCSCMatrix> st5;
@@ -1411,6 +1417,8 @@ TYPED_TEST_P(TestFloatingSparseCSFTensorEquality, TestEquality) {
   ASSERT_OK_AND_ASSIGN(st4, SparseCSFTensor::Make(*this->tensor2_));
   EXPECT_FALSE(st4->Equals(*st4));                                  // same object
   EXPECT_TRUE(st4->Equals(*st4, EqualOptions().nans_equal(true)));  // same object
+  ASSERT_OK_AND_ASSIGN(auto my_tensor,st4->ToTensor());
+  ASSERT_TRUE(my_tensor->Equals(*this->tensor2_));
 
   c_value_type values5[2][3][4][5] = {};
   std::copy_n(&this->values2_[0][0][0][0], this->length_ / sizeof(c_value_type),

[pitrou]

It depends, is the fix simple enough to be integrated here?

[andishgar]

It depends, is the fix simple enough to be integrated here?
hat casuses it

@pitrou
I’m not sure about the cause. I noticed the error while running the tests after applying your suggestion for COOTensor.
After looking into it further, I found that the same issue also affects the other sparse tensor formats.

[rok]

@andishgar how is my_tensor created in your test?

[andishgar]

@rok
Here is the complete version of the test.

TYPED_TEST_P(TestFloatingSparseCOOTensorEquality, TestEquality) {
  using ValueType = TypeParam;
  using c_value_type = typename ValueType::c_type;
  static_assert(is_floating_type<ValueType>::value, "Float type is required");

  std::shared_ptr<SparseCOOTensor> st1, st2, st3;
  ASSERT_OK_AND_ASSIGN(st1, SparseCOOTensor::Make(*this->tensor1_));
  ASSERT_OK_AND_ASSIGN(st2, SparseCOOTensor::Make(*this->tensor2_));
  ASSERT_OK_AND_ASSIGN(st3, SparseCOOTensor::Make(*this->tensor1_));

  ASSERT_TRUE(st1->Equals(*st1));
  ASSERT_FALSE(st1->Equals(*st2));
  ASSERT_TRUE(st1->Equals(*st3));

  // sparse tensors with NaNs
  const c_value_type nan_value = static_cast<c_value_type>(NAN);
  this->values2_[13] = nan_value;
  EXPECT_TRUE(std::isnan(this->tensor2_->Value({1, 0, 1})));

  std::shared_ptr<SparseCOOTensor> st4;
  ASSERT_OK_AND_ASSIGN(st4, SparseCOOTensor::Make(*this->tensor2_));
  EXPECT_FALSE(st4->Equals(*st4));                                  // same object
  EXPECT_TRUE(st4->Equals(*st4, EqualOptions().nans_equal(true)));  // same object
  ASSERT_OK_AND_ASSIGN(auto my_tensor,st4->ToTensor());
  ASSERT_TRUE(my_tensor->Equals(*this->tensor2_));

  std::vector<c_value_type> values5 = this->values2_;
  std::shared_ptr<SparseCOOTensor> st5;
  std::shared_ptr<Buffer> buffer5 = Buffer::Wrap(values5);
  NumericTensor<ValueType> tensor5(buffer5, this->shape_);
  ASSERT_OK_AND_ASSIGN(st5, SparseCOOTensor::Make(tensor5));
  EXPECT_FALSE(st4->Equals(*st5));                                  // different memory
  EXPECT_TRUE(st4->Equals(*st5, EqualOptions().nans_equal(true)));  // different memory
}

[rok]

This only happens if you have a nan in the tensor? By default nans are not considered equal.

[andishgar]

@rok You’re right — I added some extra code to handle validation, and the problem is solved.

[rok]

I'll look at this first thing tomorrow.