You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Arkur745 (via GitHub)" <gi...@apache.org> on 2025/09/07 18:59:22 UTC

[PR] feat(cpp): Add UnsafeAppend methods to StructBuilder [arrow]

Arkur745 opened a new pull request, #47522:
URL: https://github.com/apache/arrow/pull/47522

   Fixes #45722
   
   **Description:**
   This PR implements the three requested `UnsafeAppend` methods for the `StructBuilder` to improve performance in scenarios where the caller can guarantee capacity and validity. The new methods are:
   - `void UnsafeAppend()`
   - `Status UnsafeAppendNull()`
   - `Status UnsafeAppendNulls(int64_t length)`
   
   New unit tests have been added to `array_struct_test.cc` to validate the behavior of each of these new methods. All new and existing tests are passing.
   
   **Note on CI:**
   During a full `ctest` run, I observed a failure in `arrow-compute-scalar-cast-test`. I have verified that this test also fails on the base branch without my changes, confirming it is an unrelated, pre-existing issue.


-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #47522:
URL: https://github.com/apache/arrow/pull/47522#discussion_r2338294672


##########
cpp/src/arrow/array/array_struct_test.cc:
##########
@@ -748,4 +748,94 @@ TEST(TestFieldRef, GetChildren) {
   AssertArraysEqual(*a, *expected_a);
 }
 
+TEST(TestStructBuilderUnsafe, UnsafeAppend) {
+  auto int_type = int32();
+  auto str_type = utf8();
+  auto struct_type = struct_({field("a", int_type), field("b", str_type)});
+  auto pool = default_memory_pool();
+  std::shared_ptr<Array> final_array;
+  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));

Review Comment:
   ```suggestion
     ASSERT_OK_AND_ASSIGN(auto final_array, builder.Finish());
   ```



##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -791,6 +808,22 @@ class ARROW_EXPORT StructBuilder : public ArrayBuilder {
     return Status::OK();
   }
 
+  /// This method is "unsafe" because it does not check for capacity.
+  /// 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.
+  /// param length The number of null slots to append.
+  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));
+    }

Review Comment:
   ```suggestion
       for (const auto& child : children_) {
         ARROW_RETURN_NOT_OK(child->AppendEmptyValues(length));
       }
       UnsafeAppendToBitmap(length, false);
   ```



##########
cpp/src/arrow/array/array_struct_test.cc:
##########
@@ -748,4 +748,94 @@ TEST(TestFieldRef, GetChildren) {
   AssertArraysEqual(*a, *expected_a);
 }
 
+TEST(TestStructBuilderUnsafe, UnsafeAppend) {
+  auto int_type = int32();
+  auto str_type = utf8();
+  auto struct_type = struct_({field("a", int_type), field("b", str_type)});

Review Comment:
   How about using meaningful field name for readability?
   
   ```suggestion
     auto struct_type = struct_({field("int", int_type), field("str", str_type)});
   ```



##########
cpp/src/arrow/array/array_struct_test.cc:
##########
@@ -748,4 +748,94 @@ TEST(TestFieldRef, GetChildren) {
   AssertArraysEqual(*a, *expected_a);
 }
 
+TEST(TestStructBuilderUnsafe, UnsafeAppend) {
+  auto int_type = int32();
+  auto str_type = utf8();
+  auto struct_type = struct_({field("a", int_type), field("b", str_type)});
+  auto pool = default_memory_pool();
+  std::shared_ptr<Array> final_array;
+  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);
+  AssertArraysEqual(*final_array, *expected_array);

Review Comment:
   ```suggestion
     AssertArraysEqual(*expected_array, *final_array);
   ```



##########
cpp/src/arrow/array/array_struct_test.cc:
##########
@@ -748,4 +748,94 @@ TEST(TestFieldRef, GetChildren) {
   AssertArraysEqual(*a, *expected_a);
 }
 
+TEST(TestStructBuilderUnsafe, UnsafeAppend) {
+  auto int_type = int32();
+  auto str_type = utf8();
+  auto struct_type = struct_({field("a", int_type), field("b", str_type)});
+  auto pool = default_memory_pool();
+  std::shared_ptr<Array> final_array;
+  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"}])";
+

Review Comment:
   ```suggestion
   
     auto expected_json = R"([{"a": 1, "b": "hello"}, {"a": 2, "b": "arrow"}])";
   ```



-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #47522:
URL: https://github.com/apache/arrow/pull/47522#discussion_r2328951954


##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -771,6 +771,13 @@ class ARROW_EXPORT StructBuilder : public ArrayBuilder {
     return Status::OK();
   }
 
+  /// This method is "unsafe" because it does not update the null bitmap

Review Comment:
   ```suggestion
     /// This method is "unsafe" because it does not update the null bitmap.
   ```



##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -771,6 +771,13 @@ class ARROW_EXPORT StructBuilder : public ArrayBuilder {
     return Status::OK();
   }
 
+  /// 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(){

Review Comment:
   ```suggestion
     void UnsafeAppend(bool is_valid = true) {
   ```



##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -780,6 +787,19 @@ class ARROW_EXPORT StructBuilder : public ArrayBuilder {
     return Append(false);
   }
 
+  /// This method is "unsafe" because it does not check for capacity.
+  /// 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());
+    }
+    length_++;

Review Comment:
   ```suggestion
       UnsafeAppend(false);
   ```



##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -780,6 +787,19 @@ class ARROW_EXPORT StructBuilder : public ArrayBuilder {
     return Append(false);
   }
 
+  /// This method is "unsafe" because it does not check for capacity.
+  /// 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());

Review Comment:
   We need to append an empty value not null.
   
   Ah, we may need `UnsafeAppendEmptyValue()` before we work on this. @pitrou What do you think about this?



##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -771,6 +771,13 @@ class ARROW_EXPORT StructBuilder : public ArrayBuilder {
     return Status::OK();
   }
 
+  /// 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_;

Review Comment:
   ```suggestion
       UnsafeAppendToBitmap(is_valid);
   ```
   
   Could you use `UnsafeAppend(is_valid)` in `Append()` instead of `UnsafeAppendToBitmap(is_valid)`?



-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "Arkur745 (via GitHub)" <gi...@apache.org>.
Arkur745 commented on PR #47522:
URL: https://github.com/apache/arrow/pull/47522#issuecomment-3292046241

   @pitrou I'll update this PR with updated comments and  `UnsafeAppendEmptyValue`.


-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "Arkur745 (via GitHub)" <gi...@apache.org>.
Arkur745 commented on code in PR #47522:
URL: https://github.com/apache/arrow/pull/47522#discussion_r2329877791


##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -780,6 +787,19 @@ class ARROW_EXPORT StructBuilder : public ArrayBuilder {
     return Append(false);
   }
 
+  /// This method is "unsafe" because it does not check for capacity.
+  /// 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());
+    }
+    length_++;

Review Comment:
   I'm currently working on lint issues. I will commit the changes once these issues are resolved.



-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "Arkur745 (via GitHub)" <gi...@apache.org>.
Arkur745 commented on PR #47522:
URL: https://github.com/apache/arrow/pull/47522#issuecomment-3274860078

   Please checkout my recent changes. I have resolved the lint errors and it’s passing all the workflows in my fork repo.


-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #47522:
URL: https://github.com/apache/arrow/pull/47522#issuecomment-3264276290

   Could you use our PR template instead of removing it entirely?
   
   Could you fix lint failures? You can enable GitHub Actions on your fork to check CI results on your fork.


-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47522:
URL: https://github.com/apache/arrow/pull/47522#discussion_r2348886433


##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -767,10 +767,15 @@ 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.

Review Comment:
   Why "does not update the null bitmap"? Below it does.



-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47522:
URL: https://github.com/apache/arrow/pull/47522#issuecomment-3292500315

   > Or implement a basic version for now and leave the full concrete implementations for a separate PR? I think this should be in a separate PR with a separate issue.
   
   I think `UnsafeAppendEmptyValue()` and `UnsafeAppendEmptyValues()` should be implemented entirely at once.
   Either in this PR, or in a separate one.


-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat(cpp): Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #47522:
URL: https://github.com/apache/arrow/pull/47522#issuecomment-3263973896

   <!--
     Licensed to the Apache Software Foundation (ASF) under one
     or more contributor license agreements.  See the NOTICE file
     distributed with this work for additional information
     regarding copyright ownership.  The ASF licenses this file
     to you under the Apache License, Version 2.0 (the
     "License"); you may not use this file except in compliance
     with the License.  You may obtain a copy of the License at
   
       http://www.apache.org/licenses/LICENSE-2.0
   
     Unless required by applicable law or agreed to in writing,
     software distributed under the License is distributed on an
     "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
     KIND, either express or implied.  See the License for the
     specific language governing permissions and limitations
     under the License.
   -->
   
   Thanks for opening a pull request!
   
   If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
   
   Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
   
   Then could you also rename the pull request title in the following format?
   
       GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
   
   or
   
       MINOR: [${COMPONENT}] ${SUMMARY}
   
   See also:
   
     * [Other pull requests](https://github.com/apache/arrow/pulls/)
     * [Contribution Guidelines - Contributing Overview](https://arrow.apache.org/docs/developers/overview.html)
   


-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "Arkur745 (via GitHub)" <gi...@apache.org>.
Arkur745 commented on PR #47522:
URL: https://github.com/apache/arrow/pull/47522#issuecomment-3280661240

   I've updated the PR according to the template and committed the suggested changes.


-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #47522:
URL: https://github.com/apache/arrow/pull/47522#issuecomment-5713512445

   Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days.  Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.


-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #47522:
URL: https://github.com/apache/arrow/pull/47522#issuecomment-3268117276

   OK. https://github.com/apache/arrow/issues/47434 may be related. Could you rebase on main?


-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #47522:
URL: https://github.com/apache/arrow/pull/47522#issuecomment-3265928786

   Does `C:\dev\arrow\cpp\build\debug\Debug\arrow-array-test.exe` (no `--gtest_filter=*TestStructBuilder*`) work?


-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "Arkur745 (via GitHub)" <gi...@apache.org>.
Arkur745 commented on PR #47522:
URL: https://github.com/apache/arrow/pull/47522#issuecomment-3265953364

   No, its not working. all it say is
    0 tests from 0 test suites ran.
    


-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "Arkur745 (via GitHub)" <gi...@apache.org>.
Arkur745 commented on PR #47522:
URL: https://github.com/apache/arrow/pull/47522#issuecomment-3292392812

   @kou , @pitrou 
   Should I implement `UnsafeAppendEmptyValue()` and `UnsafeAppendEmptyValues()` in the base ArrayBuilder class and all the concrete builder types (Int32Builder,  StringBuilder , etc.) as part of this PR?
   This would involve:
   - Adding virtual method signatures to `ArrayBuilder `base class
   - Implementing them in all concrete builder classes
   - Then using them in my `StructBuilder` unsafe methods
   
   Or implement a basic version for now and leave the full concrete implementations for a separate PR?
   I think this should be in a separate PR with a separate issue.


-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "Arkur745 (via GitHub)" <gi...@apache.org>.
Arkur745 commented on code in PR #47522:
URL: https://github.com/apache/arrow/pull/47522#discussion_r2348908852


##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -767,10 +767,15 @@ 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.

Review Comment:
   Initially, my approach was different. But that didn't work out. I will remove this comment in my next commit.



-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "Arkur745 (via GitHub)" <gi...@apache.org>.
Arkur745 commented on code in PR #47522:
URL: https://github.com/apache/arrow/pull/47522#discussion_r2329782050


##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -780,6 +787,19 @@ class ARROW_EXPORT StructBuilder : public ArrayBuilder {
     return Append(false);
   }
 
+  /// This method is "unsafe" because it does not check for capacity.
+  /// 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());
+    }
+    length_++;

Review Comment:
   I’ve committed the changes you suggested (API updates, usage of [UnsafeAppend(is_valid)] and am currently working on resolving lint issues. I’ve also synced my branch with the latest main and am running a clean build and test cycle. I’ll update again once all CI checks pass.



-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "Arkur745 (via GitHub)" <gi...@apache.org>.
Arkur745 commented on code in PR #47522:
URL: https://github.com/apache/arrow/pull/47522#discussion_r2329782050


##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -780,6 +787,19 @@ class ARROW_EXPORT StructBuilder : public ArrayBuilder {
     return Append(false);
   }
 
+  /// This method is "unsafe" because it does not check for capacity.
+  /// 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());
+    }
+    length_++;

Review Comment:
   I’ve committed the changes you suggested (API updates, usage of [UnsafeAppend(is_valid)] and am currently working on resolving lint issues. I’ve also synced my branch with the latest main and am running a clean build and test cycle. I’ll update again once all CI checks pass.



-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "Arkur745 (via GitHub)" <gi...@apache.org>.
Arkur745 commented on PR #47522:
URL: https://github.com/apache/arrow/pull/47522#issuecomment-3265844608

   > Could you fix lint failures? You can enable GitHub Actions on your fork to check CI results on your fork.
   
   @kou, I am having the entire project and all the tests compile successfully. The final build log confirms that arrow-array-test.exe is being created correctly. 
   However, when I run the test executable directly, the Google Test runner reports that it found 0 tests, even though the code contains the new TestStructBuilder test suite.
   
   
   `C:\dev\arrow\cpp\build\debug\Debug\arrow-array-test.exe --gtest_filter=*TestStructBuilder*
   
   Running main() from C:\dev\arrow\cpp\build\_deps\googletest-src\googletest\src\gtest_main.cc
   
   Note: Google Test filter = *TestStructBuilder*
   
   [==========] Running 0 tests from 0 test suites.
   
   [==========] 0 tests from 0 test suites ran. (2 ms total)
   [  PASSED  ] 0 tests.`
   
   The core of the issue is that the build system doesn't seem to be automatically discovering the tests in array_struct_test.cc.
   I found a workaround. By manually adding this line to cpp/src/arrow/array/CMakeLists.txt:
   add_arrow_test(array_struct_test)
   `C:\dev\arrow\cpp\build> ctest -C Debug -R arrow-array-struct-test
   Test project 
   C:/dev/arrow/cpp/build
   Start 17: arrow-array-struct-test
   1/1 Test #17: arrow-array-struct-test ..........   Passed   2.98 sec`
   
   While my specific test passes, I've discovered that my change to the CMakeLists.txt file seems to break the main test suite. When I run a full ctest, the main arrow-array-test target (which I believe is supposed to contain all the array tests) now fails.
   
   Here is the log from the full run:
   `99% tests passed, 1 tests failed out of 96
   ...
   The following tests FAILED:
   1 - arrow-array-test (Failed)`


-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GH-45722: [C++] Add UnsafeAppend methods to StructBuilder [arrow]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #47522: GH-45722: [C++] Add UnsafeAppend methods to StructBuilder
URL: https://github.com/apache/arrow/pull/47522


-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org