pitrou commented on code in PR #46981:
URL: https://github.com/apache/arrow/pull/46981#discussion_r2194794609
##########
cpp/src/arrow/array/array_test.cc:
##########
@@ -4075,4 +4078,72 @@ TYPED_TEST(TestPrimitiveArray, IndexOperator) {
}
}
+class TestHalfFloatBuilder : public ::testing::Test {
+ public:
+ void VerifyValue(HalfFloatBuilder& builder, int64_t index, float expected) {
+ ASSERT_EQ(builder.GetValue(index), Float16(expected).bits());
+ ASSERT_EQ(builder.GetValue<Float16>(index), Float16(expected));
+ ASSERT_EQ(builder.GetValue<uint16_t>(index), Float16(expected).bits());
+ ASSERT_EQ(builder[index], Float16(expected).bits());
+ }
+};
+
+TEST_F(TestHalfFloatBuilder, TestAppend) {
+ HalfFloatBuilder builder;
+ ASSERT_OK(builder.Append(Float16(0.0f)));
+ ASSERT_OK(builder.Append(Float16(1.0f).bits()));
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.Reserve(3));
+ builder.UnsafeAppend(Float16(3.0f));
+ builder.UnsafeAppend(Float16(4.0f).bits());
+ builder.UnsafeAppend(uint16_t{15872}); // 1.5f
+
+ VerifyValue(builder, 0, 0.0f);
+ VerifyValue(builder, 1, 1.0f);
+ VerifyValue(builder, 3, 3.0f);
+ VerifyValue(builder, 4, 4.0f);
+ VerifyValue(builder, 5, 1.5f);
+}
+
+TEST_F(TestHalfFloatBuilder, TestBulkAppend) {
+ HalfFloatBuilder builder;
+
+ ASSERT_OK(builder.AppendValues(5, Float16(1.5)));
+ uint16_t val = Float16(2.0f).bits();
+ ASSERT_OK(builder.AppendValues({val, val, val, val}, {0, 1, 0, 1}));
+ ASSERT_EQ(builder.length(), 9);
+ for (int i = 0; i < 5; i++) {
+ VerifyValue(builder, i, 1.5f);
+ }
+ ASSERT_OK_AND_ASSIGN(auto array, builder.Finish());
+ ASSERT_EQ(array->null_count(), 2);
+ ASSERT_EQ(array->length(), 9);
+ auto comp = ArrayFromJSON(float16(), "[1.5,1.5,1.5,1.5,1.5,null,2,null,2]");
+ ASSERT_TRUE(array->Equals(*comp));
+
+ std::vector<Float16> vals = {Float16(2.5), Float16(3.5)};
+ std::vector<bool> is_valid = {true, true};
+ std::vector<uint8_t> bitmap = {1, 1};
+ ASSERT_OK(builder.AppendValues(vals));
+ ASSERT_OK(builder.AppendValues(vals, is_valid));
+ ASSERT_OK(builder.AppendValues(vals.data(), vals.size(), is_valid));
+ ASSERT_OK(builder.AppendValues(vals.data(), vals.size()));
+ ASSERT_OK(builder.AppendValues(vals.data(), vals.size(), bitmap.data(), 0));
+
+ for (int i = 0; i < 5; i++) {
+ VerifyValue(builder, (2 * i), 2.5);
+ VerifyValue(builder, (2 * i) + 1, 3.5);
+ }
+}
+
+TEST_F(TestHalfFloatBuilder, TestReinterpretCast) {
Review Comment:
This is unrelated to `HalfFloatBuilder`. If this test is useful, perhaps
move it to `arrow/util/float16_test.cc`?
##########
cpp/src/arrow/array/array_test.cc:
##########
@@ -4075,4 +4078,72 @@ TYPED_TEST(TestPrimitiveArray, IndexOperator) {
}
}
+class TestHalfFloatBuilder : public ::testing::Test {
+ public:
+ void VerifyValue(HalfFloatBuilder& builder, int64_t index, float expected) {
Review Comment:
Nit: use const-ref here
```suggestion
void VerifyValue(const HalfFloatBuilder& builder, int64_t index, float
expected) {
```
##########
cpp/src/arrow/array/array_test.cc:
##########
@@ -4075,4 +4078,72 @@ TYPED_TEST(TestPrimitiveArray, IndexOperator) {
}
}
+class TestHalfFloatBuilder : public ::testing::Test {
+ public:
+ void VerifyValue(HalfFloatBuilder& builder, int64_t index, float expected) {
+ ASSERT_EQ(builder.GetValue(index), Float16(expected).bits());
+ ASSERT_EQ(builder.GetValue<Float16>(index), Float16(expected));
+ ASSERT_EQ(builder.GetValue<uint16_t>(index), Float16(expected).bits());
+ ASSERT_EQ(builder[index], Float16(expected).bits());
+ }
+};
+
+TEST_F(TestHalfFloatBuilder, TestAppend) {
+ HalfFloatBuilder builder;
+ ASSERT_OK(builder.Append(Float16(0.0f)));
+ ASSERT_OK(builder.Append(Float16(1.0f).bits()));
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.Reserve(3));
+ builder.UnsafeAppend(Float16(3.0f));
+ builder.UnsafeAppend(Float16(4.0f).bits());
+ builder.UnsafeAppend(uint16_t{15872}); // 1.5f
+
+ VerifyValue(builder, 0, 0.0f);
+ VerifyValue(builder, 1, 1.0f);
+ VerifyValue(builder, 3, 3.0f);
+ VerifyValue(builder, 4, 4.0f);
+ VerifyValue(builder, 5, 1.5f);
+}
+
+TEST_F(TestHalfFloatBuilder, TestBulkAppend) {
+ HalfFloatBuilder builder;
+
+ ASSERT_OK(builder.AppendValues(5, Float16(1.5)));
+ uint16_t val = Float16(2.0f).bits();
+ ASSERT_OK(builder.AppendValues({val, val, val, val}, {0, 1, 0, 1}));
+ ASSERT_EQ(builder.length(), 9);
+ for (int i = 0; i < 5; i++) {
+ VerifyValue(builder, i, 1.5f);
+ }
+ ASSERT_OK_AND_ASSIGN(auto array, builder.Finish());
Review Comment:
We should validate the built array here.
```suggestion
ASSERT_OK_AND_ASSIGN(auto array, builder.Finish());
ASSERT_OK(array->ValidateFull());
```
##########
cpp/src/arrow/array/builder_primitive.h:
##########
@@ -384,6 +384,101 @@ using DurationBuilder = NumericBuilder<DurationType>;
/// @}
+class ARROW_EXPORT HalfFloatBuilder : public NumericBuilder<HalfFloatType> {
Review Comment:
Perhaps `/// \addtogroup numeric-builders` so as to inject it at the right
place in the documentation?
##########
cpp/src/arrow/array/array_test.cc:
##########
@@ -4075,4 +4078,72 @@ TYPED_TEST(TestPrimitiveArray, IndexOperator) {
}
}
+class TestHalfFloatBuilder : public ::testing::Test {
+ public:
+ void VerifyValue(HalfFloatBuilder& builder, int64_t index, float expected) {
+ ASSERT_EQ(builder.GetValue(index), Float16(expected).bits());
+ ASSERT_EQ(builder.GetValue<Float16>(index), Float16(expected));
+ ASSERT_EQ(builder.GetValue<uint16_t>(index), Float16(expected).bits());
+ ASSERT_EQ(builder[index], Float16(expected).bits());
+ }
+};
+
+TEST_F(TestHalfFloatBuilder, TestAppend) {
+ HalfFloatBuilder builder;
+ ASSERT_OK(builder.Append(Float16(0.0f)));
+ ASSERT_OK(builder.Append(Float16(1.0f).bits()));
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.Reserve(3));
+ builder.UnsafeAppend(Float16(3.0f));
+ builder.UnsafeAppend(Float16(4.0f).bits());
+ builder.UnsafeAppend(uint16_t{15872}); // 1.5f
+
+ VerifyValue(builder, 0, 0.0f);
+ VerifyValue(builder, 1, 1.0f);
+ VerifyValue(builder, 3, 3.0f);
+ VerifyValue(builder, 4, 4.0f);
+ VerifyValue(builder, 5, 1.5f);
+}
+
+TEST_F(TestHalfFloatBuilder, TestBulkAppend) {
+ HalfFloatBuilder builder;
+
+ ASSERT_OK(builder.AppendValues(5, Float16(1.5)));
+ uint16_t val = Float16(2.0f).bits();
+ ASSERT_OK(builder.AppendValues({val, val, val, val}, {0, 1, 0, 1}));
+ ASSERT_EQ(builder.length(), 9);
+ for (int i = 0; i < 5; i++) {
+ VerifyValue(builder, i, 1.5f);
+ }
+ ASSERT_OK_AND_ASSIGN(auto array, builder.Finish());
+ ASSERT_EQ(array->null_count(), 2);
+ ASSERT_EQ(array->length(), 9);
+ auto comp = ArrayFromJSON(float16(), "[1.5,1.5,1.5,1.5,1.5,null,2,null,2]");
+ ASSERT_TRUE(array->Equals(*comp));
+
+ std::vector<Float16> vals = {Float16(2.5), Float16(3.5)};
+ std::vector<bool> is_valid = {true, true};
+ std::vector<uint8_t> bitmap = {1, 1};
+ ASSERT_OK(builder.AppendValues(vals));
+ ASSERT_OK(builder.AppendValues(vals, is_valid));
+ ASSERT_OK(builder.AppendValues(vals.data(), vals.size(), is_valid));
+ ASSERT_OK(builder.AppendValues(vals.data(), vals.size()));
+ ASSERT_OK(builder.AppendValues(vals.data(), vals.size(), bitmap.data(), 0));
+
+ for (int i = 0; i < 5; i++) {
Review Comment:
It might be easier to use `ArrayFromJSON` to check the final built array
instead.
##########
cpp/src/arrow/array/array_test.cc:
##########
@@ -4075,4 +4078,72 @@ TYPED_TEST(TestPrimitiveArray, IndexOperator) {
}
}
+class TestHalfFloatBuilder : public ::testing::Test {
+ public:
+ void VerifyValue(HalfFloatBuilder& builder, int64_t index, float expected) {
+ ASSERT_EQ(builder.GetValue(index), Float16(expected).bits());
+ ASSERT_EQ(builder.GetValue<Float16>(index), Float16(expected));
+ ASSERT_EQ(builder.GetValue<uint16_t>(index), Float16(expected).bits());
+ ASSERT_EQ(builder[index], Float16(expected).bits());
+ }
+};
+
+TEST_F(TestHalfFloatBuilder, TestAppend) {
+ HalfFloatBuilder builder;
+ ASSERT_OK(builder.Append(Float16(0.0f)));
+ ASSERT_OK(builder.Append(Float16(1.0f).bits()));
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.Reserve(3));
+ builder.UnsafeAppend(Float16(3.0f));
+ builder.UnsafeAppend(Float16(4.0f).bits());
+ builder.UnsafeAppend(uint16_t{15872}); // 1.5f
+
+ VerifyValue(builder, 0, 0.0f);
+ VerifyValue(builder, 1, 1.0f);
+ VerifyValue(builder, 3, 3.0f);
+ VerifyValue(builder, 4, 4.0f);
+ VerifyValue(builder, 5, 1.5f);
+}
+
+TEST_F(TestHalfFloatBuilder, TestBulkAppend) {
+ HalfFloatBuilder builder;
+
+ ASSERT_OK(builder.AppendValues(5, Float16(1.5)));
+ uint16_t val = Float16(2.0f).bits();
+ ASSERT_OK(builder.AppendValues({val, val, val, val}, {0, 1, 0, 1}));
+ ASSERT_EQ(builder.length(), 9);
+ for (int i = 0; i < 5; i++) {
+ VerifyValue(builder, i, 1.5f);
+ }
+ ASSERT_OK_AND_ASSIGN(auto array, builder.Finish());
+ ASSERT_EQ(array->null_count(), 2);
+ ASSERT_EQ(array->length(), 9);
+ auto comp = ArrayFromJSON(float16(), "[1.5,1.5,1.5,1.5,1.5,null,2,null,2]");
+ ASSERT_TRUE(array->Equals(*comp));
Review Comment:
We should use `AssertArraysEqual` so as to get better error messages on
failure.
##########
docs/source/cpp/api/builder.rst:
##########
@@ -34,6 +34,9 @@ Primitive
.. doxygenclass:: arrow::BooleanBuilder
:members:
+.. doxygenclass:: arrow::HalfFloatBuilder
Review Comment:
This shouldn't be necessary if `HalfFloatBuilder` is added to the
`numeric-builders` group.
##########
cpp/src/arrow/array/array_test.cc:
##########
@@ -4075,4 +4078,72 @@ TYPED_TEST(TestPrimitiveArray, IndexOperator) {
}
}
+class TestHalfFloatBuilder : public ::testing::Test {
+ public:
+ void VerifyValue(HalfFloatBuilder& builder, int64_t index, float expected) {
+ ASSERT_EQ(builder.GetValue(index), Float16(expected).bits());
+ ASSERT_EQ(builder.GetValue<Float16>(index), Float16(expected));
+ ASSERT_EQ(builder.GetValue<uint16_t>(index), Float16(expected).bits());
+ ASSERT_EQ(builder[index], Float16(expected).bits());
+ }
+};
+
+TEST_F(TestHalfFloatBuilder, TestAppend) {
+ HalfFloatBuilder builder;
+ ASSERT_OK(builder.Append(Float16(0.0f)));
+ ASSERT_OK(builder.Append(Float16(1.0f).bits()));
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.Reserve(3));
+ builder.UnsafeAppend(Float16(3.0f));
+ builder.UnsafeAppend(Float16(4.0f).bits());
+ builder.UnsafeAppend(uint16_t{15872}); // 1.5f
+
+ VerifyValue(builder, 0, 0.0f);
+ VerifyValue(builder, 1, 1.0f);
+ VerifyValue(builder, 3, 3.0f);
+ VerifyValue(builder, 4, 4.0f);
+ VerifyValue(builder, 5, 1.5f);
+}
+
+TEST_F(TestHalfFloatBuilder, TestBulkAppend) {
+ HalfFloatBuilder builder;
+
+ ASSERT_OK(builder.AppendValues(5, Float16(1.5)));
+ uint16_t val = Float16(2.0f).bits();
+ ASSERT_OK(builder.AppendValues({val, val, val, val}, {0, 1, 0, 1}));
+ ASSERT_EQ(builder.length(), 9);
+ for (int i = 0; i < 5; i++) {
+ VerifyValue(builder, i, 1.5f);
+ }
+ ASSERT_OK_AND_ASSIGN(auto array, builder.Finish());
+ ASSERT_EQ(array->null_count(), 2);
+ ASSERT_EQ(array->length(), 9);
+ auto comp = ArrayFromJSON(float16(), "[1.5,1.5,1.5,1.5,1.5,null,2,null,2]");
+ ASSERT_TRUE(array->Equals(*comp));
+
+ std::vector<Float16> vals = {Float16(2.5), Float16(3.5)};
+ std::vector<bool> is_valid = {true, true};
+ std::vector<uint8_t> bitmap = {1, 1};
Review Comment:
Can we make this less trivial by having null values?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
