GH-46739: [C++] Fix Float16 signed zero/NaN equality comparisons by benibus · Pull Request #46973 · apache/arrow

benibus · 2025-07-02T04:30:47Z

Rationale for this change

Equality comparisons between half-floats (used in their scalar/array Equals methods) do not properly handle EqualOptions::nans_equal and EqualOptions::signed_zeros_equal.

What changes are included in this PR?

Internal fixes to the current comparison behavior and additional tests as needed
Prevents Float16 NaNs from being randomly generated by test utilities by default (matching behavior for float/double)

Are these changes tested?

Yes

Are there any user-facing changes?

No

GitHub Issue: [C++] Incorrect Float16 Comparison for NaN and signed zero #46739

github-actions · 2025-07-02T04:31:12Z

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

github-actions · 2025-07-04T00:17:16Z

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

github-actions · 2025-07-05T01:26:19Z

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

pitrou

Thanks for doing this @benibus . This is a useful fix, here are a couple comments and suggestions.

cpp/src/arrow/array/array_test.cc

cpp/src/arrow/scalar_test.cc

cpp/src/arrow/testing/random.cc

pitrou · 2025-08-21T12:13:33Z

@benibus Is this ready for review again?

benibus · 2025-08-22T05:54:56Z

@pitrou Yes, sorry. Feel free to take another look.

pitrou

LGTM in general, some additional comments below

pitrou · 2025-08-26T14:05:26Z

cpp/src/arrow/testing/random.cc

This doesn't fix GenerateTypedData when nan_probability_ is non-zero (it will use std::numeric_limits<uint16_t>::quiet_NaN() which is 0 and translates to Float16(0.0)).

pitrou · 2025-08-26T14:07:48Z

cpp/src/arrow/testing/random.cc

DistributionType will be std::uniform_int_distribution<uint16_t> which will certainly not respect the min and max values once translated to Float16?

Perhaps ValueType needs to be Float16 here to make sure we don't misuse uint16_t like this, and DistributionType could be ::arrow::random::uniform_real_distribution<float>.

pitrou · 2025-08-26T14:14:37Z

cpp/src/arrow/testing/gtest_util.h

I wouldn't expect these checks in a helper function, especially as we supposedly have unit tests for this already (otherwise we should add them).

pitrou · 2025-08-26T14:17:06Z

cpp/src/arrow/scalar.h

Perhaps use std::decay_t instead of std::remove_reference_t?

pitrou · 2025-08-26T14:18:28Z

cpp/src/arrow/scalar_test.cc

We should probably keep the check here by using if constexpr as below?

pitrou · 2025-08-26T14:18:56Z

cpp/src/arrow/scalar_test.cc

Since we are changing this, perhaps ASSERT_TRUE(set.emplace(...).second) would be better?

Temporary for now

Since github.com/apache/pull/46981, HalfFloatBuilder now accepts Float16 values, making RealToCType's usage unnecessary in several places.

pitrou

LGTM, just one last thing

pitrou · 2025-09-04T06:00:26Z

cpp/src/arrow/testing/random.cc

+      ARROW_LOG(INFO) << "min = " << min_value.ToFloat();
+      ARROW_LOG(INFO) << "max = " << max_value.ToFloat();


You probably mean to remove these :)

Ah, thanks! Just removed them.

pitrou · 2025-09-04T08:32:21Z

@github-actions crossbow submit -g cpp

github-actions · 2025-09-04T08:34:54Z

Revision: 116b975

Submitted crossbow builds: ursacomputing/crossbow @ actions-868dcfc65b

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-42-cpp
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-bundled
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

pitrou · 2025-09-04T09:23:40Z

The Valgrind failure is unrelated, see #47496

pitrou · 2025-09-04T09:24:18Z

Thanks a lot @benibus !

conbench-apache-arrow · 2025-09-04T16:18:47Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit caf4f70.

There weren't enough matching historic benchmark results to make a call on whether there were regressions.

The full Conbench report has more details.

apache#46973) ### Rationale for this change Equality comparisons between half-floats (used in their scalar/array `Equals` methods) do not properly handle `EqualOptions::nans_equal` and `EqualOptions::signed_zeros_equal`. ### What changes are included in this PR? - Internal fixes to the current comparison behavior and additional tests as needed - Prevents Float16 NaNs from being randomly generated by test utilities by default (matching behavior for float/double) ### Are these changes tested? Yes ### Are there any user-facing changes? No * GitHub Issue: apache#46739 Authored-by: Benjamin Harkins <benpharkins@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>

github-actions bot added Component: C++ awaiting review Awaiting review labels Jul 2, 2025

benibus marked this pull request as ready for review July 5, 2025 01:27

benibus mentioned this pull request Jul 5, 2025

[C++] Add tests for HalfFloatScalar #46893

Open

pitrou requested changes Jul 8, 2025

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 8, 2025

benibus force-pushed the GH-46739-incorrect-float16-compare branch from 9cb6072 to f6b74b2 Compare July 11, 2025 01:30

pitrou requested changes Aug 26, 2025

View reviewed changes

benibus force-pushed the GH-46739-incorrect-float16-compare branch from f6b74b2 to 866f533 Compare September 2, 2025 01:29

benibus added 14 commits September 3, 2025 21:04

Add example tests mentioned in issue

6195178

Temporary for now

Fix Float16 zero/NaN comparisons

93b8a0a

Exclude Float16 NaNs from being randomly generated

187877b

Update scalar/array tests

3ee9649

Move/revise GetFloat test helper

42cc849

Enable constructing HalfFloatScalar from Float16

7a730c5

Test HalfFloat in TestNumericScalar

08d45d1

Fix params for RandomArrayGenerator::Float16

8c8ad06

Use is_half_float_type

e3a3688

Fix MSVC warnings

109fe8c

Improve random HalfFloat generation

4e69d1c

Address additional review points

2e53f4a

Remove RealToCType

d038d96

Since github.com/apache/pull/46981, HalfFloatBuilder now accepts Float16 values, making RealToCType's usage unnecessary in several places.

Replace NumericHelper

fb60726

benibus force-pushed the GH-46739-incorrect-float16-compare branch from 866f533 to fb60726 Compare September 4, 2025 01:09

benibus requested a review from pitrou September 4, 2025 02:05

pitrou approved these changes Sep 4, 2025

View reviewed changes

Remove leftover debug logs

116b975

pitrou merged commit caf4f70 into apache:main Sep 4, 2025
39 checks passed

pitrou removed the awaiting committer review Awaiting committer review label Sep 4, 2025

pitrou mentioned this pull request Sep 4, 2025

[C++] Incorrect Float16 Comparison for NaN and signed zero #46739

Closed

		ARROW_LOG(INFO) << "min = " << min_value.ToFloat();
		ARROW_LOG(INFO) << "max = " << max_value.ToFloat();

Conversation

benibus commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Jul 2, 2025

Uh oh!

github-actions bot commented Jul 4, 2025

Uh oh!

github-actions bot commented Jul 5, 2025

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou commented Aug 21, 2025

Uh oh!

benibus commented Aug 22, 2025

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Sep 4, 2025

Uh oh!

github-actions bot commented Sep 4, 2025

Uh oh!

pitrou commented Sep 4, 2025

Uh oh!

Uh oh!

pitrou commented Sep 4, 2025

Uh oh!

conbench-apache-arrow bot commented Sep 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

benibus commented Jul 2, 2025 •

edited

Loading