Skip to content

Commit

Permalink
apacheGH-41089: [C++] Clean up remaining tasks related to half float …
Browse files Browse the repository at this point in the history
…casts (apache#41084)

### Rationale for this change

Address remaining tasks brought up post-merge in this PR: apache#40067

### What changes are included in this PR?

Adds a couple of tests and cleans up cruft on another test.

### Are these changes tested?

Yes, as these are test-only related changes.

### Are there any user-facing changes?

No.

* GitHub Issue: apache#41089 

Authored-by: Clif Houck <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
  • Loading branch information
ClifHouck authored and rok committed May 8, 2024
1 parent 4e7e58b commit 7ec273c
Showing 1 changed file with 4 additions and 26 deletions.
30 changes: 4 additions & 26 deletions cpp/src/arrow/ipc/json_simple_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -189,21 +189,6 @@ class TestIntegers : public ::testing::Test {

TYPED_TEST_SUITE_P(TestIntegers);

template <typename DataType>
std::vector<typename DataType::c_type> TestIntegersMutateIfNeeded(
std::vector<typename DataType::c_type> data) {
return data;
}

// TODO: This works, but is it the right way to do this?
template <>
std::vector<HalfFloatType::c_type> TestIntegersMutateIfNeeded<HalfFloatType>(
std::vector<HalfFloatType::c_type> data) {
std::for_each(data.begin(), data.end(),
[](HalfFloatType::c_type& value) { value = Float16(value).bits(); });
return data;
}

TYPED_TEST_P(TestIntegers, Basics) {
using T = TypeParam;
using c_type = typename T::c_type;
Expand All @@ -212,17 +197,16 @@ TYPED_TEST_P(TestIntegers, Basics) {
auto type = this->type();

AssertJSONArray<T>(type, "[]", {});
AssertJSONArray<T>(type, "[4, 0, 5]", TestIntegersMutateIfNeeded<T>({4, 0, 5}));
AssertJSONArray<T>(type, "[4, null, 5]", {true, false, true},
TestIntegersMutateIfNeeded<T>({4, 0, 5}));
AssertJSONArray<T>(type, "[4, 0, 5]", {4, 0, 5});
AssertJSONArray<T>(type, "[4, null, 5]", {true, false, true}, {4, 0, 5});

// Test limits
const auto min_val = std::numeric_limits<c_type>::min();
const auto max_val = std::numeric_limits<c_type>::max();
std::string json_string = JSONArray(0, 1, min_val);
AssertJSONArray<T>(type, json_string, TestIntegersMutateIfNeeded<T>({0, 1, min_val}));
AssertJSONArray<T>(type, json_string, {0, 1, min_val});
json_string = JSONArray(0, 1, max_val);
AssertJSONArray<T>(type, json_string, TestIntegersMutateIfNeeded<T>({0, 1, max_val}));
AssertJSONArray<T>(type, json_string, {0, 1, max_val});
}

TYPED_TEST_P(TestIntegers, Errors) {
Expand Down Expand Up @@ -289,12 +273,6 @@ INSTANTIATE_TYPED_TEST_SUITE_P(TestUInt8, TestIntegers, UInt8Type);
INSTANTIATE_TYPED_TEST_SUITE_P(TestUInt16, TestIntegers, UInt16Type);
INSTANTIATE_TYPED_TEST_SUITE_P(TestUInt32, TestIntegers, UInt32Type);
INSTANTIATE_TYPED_TEST_SUITE_P(TestUInt64, TestIntegers, UInt64Type);
// FIXME: I understand that HalfFloatType is backed by a uint16_t, but does it
// make sense to run this test over it?
// The way ConvertNumber for HalfFloatType is currently written, it allows the
// conversion of floating point notation to a half float, which causes failures
// in this test, one example is asserting 0.0 cannot be parsed as a half float.
// INSTANTIATE_TYPED_TEST_SUITE_P(TestHalfFloat, TestIntegers, HalfFloatType);

template <typename T>
class TestStrings : public ::testing::Test {
Expand Down

0 comments on commit 7ec273c

Please sign in to comment.