Made suggested changes to UnsafeAppend() and its related functions

Author: Arkur745

cpp/src/arrow/array/array_struct_test.cc

@@ -748,8 +748,7 @@ TEST(TestFieldRef, GetChildren) {
   AssertArraysEqual(*a, *expected_a);
 }
 
-TEST(TestStructBuilder, UnsafeAppend) {
-  
+TEST(TestStructBuilderUnsafe, UnsafeAppend) {
   auto int_type = int32();
   auto str_type = utf8();
   auto struct_type = struct_({field("a", int_type), field("b", str_type)});
@@ -758,25 +757,27 @@ TEST(TestStructBuilder, UnsafeAppend) {
   auto int_builder = std::make_shared<Int32Builder>(pool);
   auto str_builder = std::make_shared<StringBuilder>(pool);
   StructBuilder builder(struct_type, pool, {int_builder, str_builder});
-  
+
+  ASSERT_OK(builder.Reserve(2));
+
   builder.UnsafeAppend();
   ASSERT_OK(int_builder->Append(1));
   ASSERT_OK(str_builder->Append("hello"));
 
   builder.UnsafeAppend();
   ASSERT_OK(int_builder->Append(2));
   ASSERT_OK(str_builder->Append("arrow"));
-  
+
   ASSERT_OK(builder.Finish(&final_array));
   ASSERT_EQ(2, final_array->length());
   ASSERT_EQ(0, final_array->null_count());
   auto expected_json = R"([{"a": 1, "b": "hello"}, {"a": 2, "b": "arrow"}])";
+
   auto expected_array = ArrayFromJSON(struct_type, expected_json);
-  ASSERT_TRUE(final_array->Equals(expected_array));
+  AssertArraysEqual(*final_array, *expected_array);
 }
 
-TEST(TestStructBuilder, UnsafeAppendNull) {
-  
+TEST(TestStructBuilderUnsafe, UnsafeAppendNull) {
   auto int_type = int32();
   auto str_type = utf8();
   auto struct_type = struct_({field("a", int_type), field("b", str_type)});
@@ -786,6 +787,8 @@ TEST(TestStructBuilder, UnsafeAppendNull) {
   auto str_builder = std::make_shared<StringBuilder>(pool);
   StructBuilder builder(struct_type, pool, {int_builder, str_builder});
 
+  ASSERT_OK(builder.Reserve(3));
+
   builder.UnsafeAppend();
   ASSERT_OK(int_builder->Append(1));
   ASSERT_OK(str_builder->Append("hello"));
@@ -801,12 +804,12 @@ TEST(TestStructBuilder, UnsafeAppendNull) {
   ASSERT_EQ(1, final_array->null_count());
   ASSERT_TRUE(final_array->IsNull(1));
   auto expected_json = R"([{"a": 1, "b": "hello"}, null, {"a": 2, "b": "arrow"}])";
+
   auto expected_array = ArrayFromJSON(struct_type, expected_json);
-  ASSERT_TRUE(final_array->Equals(expected_array));
+  AssertArraysEqual(*final_array, *expected_array);
 }
 
-TEST(TestStructBuilder, UnsafeAppendNulls) {
-  
+TEST(TestStructBuilderUnsafe, UnsafeAppendNulls) {
   auto int_type = int32();
   auto str_type = utf8();
   auto struct_type = struct_({field("a", int_type), field("b", str_type)});
@@ -816,6 +819,8 @@ TEST(TestStructBuilder, UnsafeAppendNulls) {
   auto str_builder = std::make_shared<StringBuilder>(pool);
   StructBuilder builder(struct_type, pool, {int_builder, str_builder});
 
+  ASSERT_OK(builder.Reserve(4));
+
   builder.UnsafeAppend();
   ASSERT_OK(int_builder->Append(1));
   ASSERT_OK(str_builder->Append("hello"));
@@ -827,13 +832,10 @@ TEST(TestStructBuilder, UnsafeAppendNulls) {
   ASSERT_OK(str_builder->Append("arrow"));
 
   ASSERT_OK(builder.Finish(&final_array));
-  ASSERT_EQ(4, final_array->length());
-  ASSERT_EQ(2, final_array->null_count());
-  ASSERT_TRUE(final_array->IsNull(1));
-  ASSERT_TRUE(final_array->IsNull(2));
-  auto expected_json = R"([{"a": 1, "b": "hello"}, null, null, {"a": 2, "b": "arrow"}])";
-  auto expected_array = ArrayFromJSON(struct_type, expected_json);
-  ASSERT_TRUE(final_array->Equals(expected_array));
+
+  auto expected_array = ArrayFromJSON(
+      struct_type, R"([{"a": 1, "b": "hello"}, null, null, {"a": 2, "b": "arrow"}])");
+  AssertArraysEqual(*final_array, *expected_array);
 }
 
-} // namespace arrow
+}  // namespace arrow

cpp/src/arrow/array/builder_nested.h

@@ -767,16 +767,14 @@ class ARROW_EXPORT StructBuilder : public ArrayBuilder {
   /// be called independently to maintain data-structure consistency.
   Status Append(bool is_valid = true) {
     ARROW_RETURN_NOT_OK(Reserve(1));
-    UnsafeAppendToBitmap(is_valid);
+    UnsafeAppend(is_valid);
     return Status::OK();
   }
 
-  /// This method is "unsafe" because it does not update the null bitmap
+  /// This method is "unsafe" because it does not update the null bitmap.
   /// It doesn't check the capacity. Caller is responsible for calling Reserve()
   /// beforehand.
-  void UnsafeAppend(){
-    ++length_;
-  }
+  void UnsafeAppend(bool is_valid = true) { UnsafeAppendToBitmap(is_valid); }
 
   /// \brief Append a null value. Automatically appends an empty value to each child
   /// builder.
@@ -791,12 +789,11 @@ class ARROW_EXPORT StructBuilder : public ArrayBuilder {
   /// The caller is responsible for calling Reserve() beforehand to ensure
   /// there is enough space. This method will append nulls to all child
   /// builders to maintain a consistent state.
-  Status UnsafeAppendNull(){
-    null_bitmap_builder_.UnsafeAppend(false);
-    for(const auto& child: children_){
-      ARROW_RETURN_NOT_OK(child->AppendNull());
+  Status UnsafeAppendNull() {
+    UnsafeAppend(false);
+    for (const auto& child : children_) {
+      ARROW_RETURN_NOT_OK(child->AppendEmptyValue());
     }
-    length_++;
     return Status::OK();
   }
 
@@ -816,12 +813,14 @@ class ARROW_EXPORT StructBuilder : public ArrayBuilder {
   /// there is enough space. This method will append nulls to all child
   /// builders to maintain a consistent state.
   /// param length The number of null slots to append.
-  Status UnsafeAppendNulls(int64_t length){
-    null_bitmap_builder_.UnsafeAppend(length, false);
-    for(const auto& child:children_){
-      ARROW_RETURN_NOT_OK(child->AppendNulls(length));
+  Status UnsafeAppendNulls(int64_t length) {
+    for (int64_t i = 0; i < length; ++i) {
+      UnsafeAppend(false);
+    }
+
+    for (const auto& child : children_) {
+      ARROW_RETURN_NOT_OK(child->AppendEmptyValues(length));
     }
-    ++length_;
     return Status::OK();
   }