GH-45722: [C++] Add UnsafeAppend methods to StructBuilder #47522

Arkur745 · 2025-09-07T18:59:19Z

Rationale for this change

StructBuilder class was missing UnsafeAppend() , UnsafeAppendNull(), UnsafeAppendNulls() methods that were standard to other builders.

What changes are included in this PR?

Added three new public methods to the StructBuilder class in cpp/src/arrow/array/builder_nested.h:

void UnsafeAppend()

Status UnsafeAppendNull()

Status UnsafeAppendNulls(int64_t length)

Refactored the existing Append() methods to use a centralized private UnsafeAppend(bool is_valid) function, improving maintainability and reducing code duplication, based on reviewer feedback.

Are these changes tested?

Yes. A new test suite, TestStructBuilderUnsafe, has been added to cpp/src/arrow/array/array_struct_test.cc. This suite contains three new unit tests, one for each of the new unsafe methods, which validate their correctness and handling of nulls. All new and existing tests are passing locally.

Are there any user-facing changes?

Yes, this PR introduces new public methods to the StructBuilder API. The changes are purely additive and do not alter or remove any existing functionality, so they are not breaking.

GitHub Issue: [C++] StructBuilder should have UnsafeAppend methods #45722

github-actions · 2025-09-07T18:59:44Z

Thanks for opening a pull request!

If this is not a minor PR. 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 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:

kou · 2025-09-08T01:23:57Z

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.

kou · 2025-09-08T01:26:25Z

cpp/src/arrow/array/builder_nested.h

Suggested change

/// This method is "unsafe" because it does not update the null bitmap

/// This method is "unsafe" because it does not update the null bitmap.

kou · 2025-09-08T01:26:37Z

cpp/src/arrow/array/builder_nested.h

Suggested change

void UnsafeAppend(){

void UnsafeAppend(bool is_valid = true) {

kou · 2025-09-08T01:26:51Z

cpp/src/arrow/array/builder_nested.h

Suggested change

++length_;

UnsafeAppendToBitmap(is_valid);

Could you use UnsafeAppend(is_valid) in Append() instead of UnsafeAppendToBitmap(is_valid)?

kou · 2025-09-08T01:31:59Z

cpp/src/arrow/array/builder_nested.h

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?

kou · 2025-09-08T01:32:21Z

cpp/src/arrow/array/builder_nested.h

Suggested change

length_++;

UnsafeAppend(false);

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

Arkur745 · 2025-09-08T11:23:43Z

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)

kou · 2025-09-08T11:49:28Z

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

Arkur745 · 2025-09-08T11:57:11Z

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

kou · 2025-09-08T21:40:33Z

OK. #47434 may be related. Could you rebase on main?

Arkur745 · 2025-09-10T12:53:48Z

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

kou

Could you use our PR template instead of removing it entirely?

cpp/src/arrow/array/builder_nested.h

cpp/src/arrow/array/array_struct_test.cc

Arkur745 · 2025-09-11T13:23:22Z

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

kou

+1

@pitrou I think that we should add UnsafeAppendEmptyValue() before this. What do you think about it?

pitrou · 2025-09-15T12:46:07Z

cpp/src/arrow/array/builder_nested.h

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

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

pitrou

@kou is right, we should add UnsafeAppendEmptyValue. This can be done in this PR, or in a separate one.

Arkur745 · 2025-09-15T12:57:57Z

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

Co-authored-by: Sutou Kouhei <[email protected]>

Arkur745 · 2025-09-15T14:14:52Z

@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.

pitrou · 2025-09-15T14:34:21Z

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.

Note you can recycle the existing implementations of AppendEmptyValue and AppendEmptyValues.
Then these methods can become something like:

Status ArrayBuilder::AppendEmptyValue() {
  RETURN_NOT_OK(Reserve(1));
  UnsafeAppendEmptyValue();
  return Status::OK();
}

Status ArrayBuilder::AppendEmptyValues(int64_t length) {
  RETURN_NOT_OK(Reserve(length));
  UnsafeAppendEmptyValues(length);
  return Status::OK();
}

github-actions bot added Component: C++ awaiting review Awaiting review labels Sep 7, 2025

kou changed the title ~~feat(cpp): Add UnsafeAppend methods to StructBuilder~~ GH-45722: [C++] Add UnsafeAppend methods to StructBuilder Sep 8, 2025

kou reviewed Sep 8, 2025

View reviewed changes

Arkur745 force-pushed the ARROW-45722-unsafe-append-structbuilder branch 2 times, most recently from c629dca to a8b8faf Compare September 8, 2025 10:22

Arkur745 force-pushed the ARROW-45722-unsafe-append-structbuilder branch 2 times, most recently from 1020ebc to 5bc5f08 Compare September 8, 2025 14:12

Arkur745 force-pushed the ARROW-45722-unsafe-append-structbuilder branch 7 times, most recently from f3873a6 to 9d3abe5 Compare September 10, 2025 11:01

kou reviewed Sep 11, 2025

View reviewed changes

kou requested a review from pitrou September 11, 2025 01:40

kou approved these changes Sep 12, 2025

View reviewed changes

pitrou reviewed Sep 15, 2025

View reviewed changes

pitrou requested changes Sep 15, 2025

View reviewed changes

feat(cpp): Add UnsafeAppend methods to StructBuilder

17c3b07

Arkur745 and others added 2 commits September 15, 2025 18:40

Made suggested changes to UnsafeAppend() and its related functions

a48b6f9

Apply suggestions from code review

12d9321

Co-authored-by: Sutou Kouhei <[email protected]>

fix(cpp): Apply review feedback and fix CI failures

bf73f86

Arkur745 force-pushed the ARROW-45722-unsafe-append-structbuilder branch from f56cffb to bf73f86 Compare September 15, 2025 14:17

	/// This method is "unsafe" because it does not update the null bitmap
	/// This method is "unsafe" because it does not update the null bitmap.

	void UnsafeAppend(){
	void UnsafeAppend(bool is_valid = true) {

GH-45722: [C++] Add UnsafeAppend methods to StructBuilder #47522

Are you sure you want to change the base?

GH-45722: [C++] Add UnsafeAppend methods to StructBuilder #47522

Uh oh!

Conversation

Arkur745 commented Sep 7, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Sep 7, 2025

Uh oh!

kou commented Sep 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Arkur745 commented Sep 8, 2025 • edited by kou Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kou commented Sep 8, 2025

Uh oh!

Arkur745 commented Sep 8, 2025

Uh oh!

kou commented Sep 8, 2025

Uh oh!

Arkur745 commented Sep 10, 2025

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Arkur745 commented Sep 11, 2025

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Arkur745 commented Sep 15, 2025

Uh oh!

Arkur745 commented Sep 15, 2025

Uh oh!

pitrou commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Arkur745 commented Sep 7, 2025 •

edited by github-actions bot

Loading

Arkur745 commented Sep 8, 2025 •

edited by kou

Loading

pitrou left a comment •

edited

Loading

pitrou commented Sep 15, 2025 •

edited

Loading