Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++] During unit testing, the float array contains nan equality judgment #38624

Closed
Light-City opened this issue Nov 7, 2023 · 5 comments · Fixed by #38642
Closed

[C++] During unit testing, the float array contains nan equality judgment #38624

Light-City opened this issue Nov 7, 2023 · 5 comments · Fixed by #38642
Assignees
Milestone

Comments

@Light-City
Copy link
Contributor

Light-City commented Nov 7, 2023

Describe the bug, including details regarding any error messages, version, and platform.

The default array options now have a nan comparison of false, but in a unit test situation we might expect nan=true.
for example:

  auto schema = ::arrow::schema({
      {field("a", float32())},
      {field("b", float64())},
  });

  std::vector<std::string> expected = {R"([{"a": 1,    "b": 5},
                                           {"a": 1,    "b": 3},
                                           {"a": 3,    "b": null},
                                           {"a": NaN,  "b": 5},
                                           {"a": NaN,  "b": NaN},
                                           {"a": NaN,   "b": null},
                                           {"a": null, "b": 5},
                                           {"a": null, "b": null}
                                          ])"};
  std::vector<std::string> actual = {R"([{"a": 1,    "b": 5},
                                         {"a": 1,    "b": 3},
                                         {"a": 3,    "b": null},
                                         {"a": NaN,  "b": 5},
                                         {"a": NaN,  "b": NaN},
                                         {"a": NaN,   "b": null},
                                         {"a": null, "b": 5},
                                         {"a": null, "b": null}
                                        ])"};
  ASSERT_TABLES_EQUAL(*TableFromJSON(schema, expected), *TableFromJSON(schema, actual));

Now the test will fail.

The reasons were found to be as follows:

  bool Equals(const Array& arr, const EqualOptions& = EqualOptions::Defaults()) const;
  bool Equals(const std::shared_ptr<Array>& arr,
              const EqualOptions& = EqualOptions::Defaults()) const;

So how can I modify the interface to support this function?

since ASSERT_TABLES_EQUAL will invoke ChunkedArray::ApproxEquals funciton.There is no way to pass options here

  return internal::ApplyBinaryChunked(
             *this, other,
             [&](const Array& left_piece, const Array& right_piece,
                 int64_t ARROW_ARG_UNUSED(position)) {
               if (!left_piece.ApproxEquals(right_piece, equal_options)) {
                 return Status::Invalid("Unequal piece");
               }
               return Status::OK();
             })
      .ok();

Component(s)

C++

@Light-City Light-City changed the title Float Array Equals for nan. [C++] Float Array Equals for nan. Nov 7, 2023
@mapleFU
Copy link
Member

mapleFU commented Nov 7, 2023

EqualOptions supports a nans_equal flag which would be helpful in this case.

@Light-City
Copy link
Contributor Author

Sorry, I know this parameter, but I can't pass it during unit testing, such as ASSERT_TABLES_EQUAL

@Light-City Light-City changed the title [C++] Float Array Equals for nan. [C++] During unit testing, the float array contains nan equality judgment Nov 7, 2023
@js8544
Copy link
Collaborator

js8544 commented Nov 8, 2023

Yeah I think it's a bug because when comparing arrays we use the TestingEqualOptions() which has nans_equal=true. Could you submit a PR to fix this?

@mapleFU
Copy link
Member

mapleFU commented Nov 8, 2023

I agree it's a bug, lets fixing this

@Light-City
Copy link
Contributor Author

right.

bkietz pushed a commit that referenced this issue Nov 15, 2023
…8642)

### Rationale for this change

some other interfaces lack equal options, suck as nan equal, so we add TestingEqualOptions for each function and add some testing.

### What changes are included in this PR?

gtest_util related.

### Are these changes tested?

gtest_util.cc

### Are there any user-facing changes?
yes.
* Closes: #38624

Authored-by: light-city <455954986@qq.com>
Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>
@bkietz bkietz added this to the 15.0.0 milestone Nov 15, 2023
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
…s. (apache#38642)

### Rationale for this change

some other interfaces lack equal options, suck as nan equal, so we add TestingEqualOptions for each function and add some testing.

### What changes are included in this PR?

gtest_util related.

### Are these changes tested?

gtest_util.cc

### Are there any user-facing changes?
yes.
* Closes: apache#38624

Authored-by: light-city <455954986@qq.com>
Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants