[C++] Update the version of GoogleTest in cpp/thirdparty/versions.txt to 1.17 to prevent potential undefined behavior in ASSERT_DOUBLE_EQ and ASSERT_FLOAT_EQ

[andishgar]

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

As mentioned here, Arrow CMake currently uses version 1.16 of GoogleTest. However, as noted in this commit, prior to version 1.17, GoogleTest used union-based type punning, which is considered undefined behavior in C++ (see cppreference):

It is undefined behavior to read from the member of a union that wasn't most recently written.

Therefore, the version of GoogleTest used by Arrow should be updated to 1.17 to avoid this UB.

Component(s)

C++

[andishgar]

Dear @kou, could you verify my observation? By the way, I set the label incorrectly to cpp and I don’t know how to change it.

[kou]

Oh, I didn't know it.

I don't think that we have a problem caused by this but we can update bundled GoogleTest.
In general, we should keep bundled dependencies up to date.

The "C++" label is suitable label. "Dev" is used for language independent developer tools such as pre-commit and merge script.

[andishgar]

@kou, another issue (this is not related to the title of the issue):

I think there is little point in using the GTest macros ASSERT_DOUBLE_EQ or ASSERT_FLOAT_EQ, as their implementation relies on a constant ULP distance of 4. For reference, see the links below:

https://github.com/google/googletest/blob/1b96fa13f549387b7549cc89e1a785cf143a1a50/googletest/include/gtest/internal/gtest-internal.h#L341

https://github.com/google/googletest/blob/1b96fa13f549387b7549cc89e1a785cf143a1a50/googletest/include/gtest/internal/gtest-internal.h#L285

On the other hand, I found in this blog (note that the blog discusses ULP error, which is almost equivalent to ULP distance, with some exceptions around powers of two) that the correct ULP value for determining whether two numbers are equal depends entirely on the algorithm.

Given these reasons, shouldn’t we consider replacing ASSERT_DOUBLE_EQ and ASSERT_FLOAT_EQ in Arrow with more robust tools?

[kou]

It seems that our current tests don't use ASSERT_DOUBLE_EQ()/ASSERT_FLOAT_EQ() for robust comparison. So I feel that we can keep using ASSERT_DOUBLE_EQ()/ASSERT_FLOAT_EQ().

Do you think that we need more robust comparison for existing ASSERT_DOUBLE_EQ()/ASSERT_FLOAT_EQ()?

[andishgar]

I found some code in cpp/src/arrow/util/decimal_test.cc and cpp/src/parquet/page_index_test.cc. However, I’m not familiar with this part of the codebase and need to study it more before giving my final view.
Please note that my reply may take a few weeks, as I’m currently busy with university admission.

[kou]

OK. No problem.

[kou]

Issue resolved by pull request 48377
#48377