GH-46207: [C++] Rename arrow::util::StringBuilder and move to internal namespace #46813

Ziy1-Tan · 2025-06-14T00:49:59Z

Rationale for this change

Move and Rename function arrow::util::StringBuilder to internal namespace to avoid confusion with class arrow::StringBuilder

What changes are included in this PR?

Move arrow::util::StringBuilder to arrow::internal::StringBuilder
Rename arrow::internal::StringBuilder to arrow::internal::JoinToString
Rename arrow/util/string_builder.{h|cc} to arrow/util/string_util_internal.{h|cc}

Are these changes tested?

Yes.

Are there any user-facing changes?

No. they are used internally.

GitHub Issue: [C++] Rename arrow::util::StringBuilder and move to internal namespace #46207

raulcd

Thanks for tackling the work.
Part of the current problem on CI is with calling the file *_internal.h . The header file won't be installed and this is used on some of the bindings which requires it. We can move the namespace to arrow::internal but keep installing the header file, probably naming the files string_util.cc and string_util.h?

Ziy1-Tan · 2025-06-16T14:02:05Z

Thanks for tackling the work. Part of the current problem on CI is with calling the file *_internal.h . The header file won't be installed and this is used on some of the bindings which requires it. We can move the namespace to arrow::internal but keep installing the header file, probably naming the files string_util.cc and string_util.h?

Thank you for your review. Let me take a look.

pitrou

Thanks for doing this. Just one suggestion, otherwise LGTM.

pitrou · 2025-06-18T11:59:29Z

cpp/src/arrow/util/string_util.h


+namespace internal {
 namespace detail {


The detail namespace is now superfluous given there's already an enclosing internal namespace.

raulcd

Thanks for the PR. This looks good to me. @pitrou do you want to take a look? It seems your suggestion has been tackled.

Signed-off-by: Ziy1-Tan <[email protected]>

pitrou · 2025-07-01T08:21:56Z

@github-actions crossbow submit -g cpp

pitrou

+1, thank you for your contribution @Ziy1-Tan . I'll wait for CI to pass and then merge.

github-actions · 2025-07-01T08:24:29Z

Revision: b9c5e4d

Submitted crossbow builds: ursacomputing/crossbow @ actions-44ea48c19a

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-bundled
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

conbench-apache-arrow · 2025-07-01T17:19:01Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 1202d79.

There was 1 benchmark result indicating a performance regression:

Commit Run on amd64-m5-4xlarge-linux at 2025-07-01 12:34:45Z
- partitioned-dataset-filter (R) with dataset=dataset-taxi-parquet, language=R, query=vignette

The full Conbench report has more details.

kou · 2025-07-04T04:27:23Z

@github-actions crossbow submit test-r-rstudio-r-base-4.1-opensuse155

github-actions · 2025-07-04T04:29:36Z

Revision: b9c5e4d

Submitted crossbow builds: ursacomputing/crossbow @ actions-21818136da

Task	Status
test-r-rstudio-r-base-4.1-opensuse155

) ### Rationale for this change OpenSUSE 15.5 ships old GCC (7.5) that doesn't have enough C++17 support. ### What changes are included in this PR? Use Ubuntu 20.04 that ships GCC 9.3 instead of OpenSUSE 15.5. Ubuntu 20.04 reached EOL but we can use it for now. We discussed why we need OpenSUSE 15.5 based job at #45718 (comment) . We have the job because https://arrow.apache.org/docs/developers/cpp/building.html said "gcc 7.1 and higher should be sufficient". We need require GCC 9 or later with #46813. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: #46989 Lead-authored-by: Sutou Kouhei <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

Ziy1-Tan requested review from wgtmac and lidavidm as code owners June 14, 2025 00:50

github-actions bot added Component: Parquet Component: C++ Component: FlightRPC awaiting review Awaiting review labels Jun 14, 2025

raulcd reviewed Jun 16, 2025

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jun 16, 2025

Ziy1-Tan force-pushed the string_builder branch from 8cbbd43 to d7f8136 Compare June 16, 2025 14:15

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 16, 2025

Ziy1-Tan force-pushed the string_builder branch from d7f8136 to fdc7868 Compare June 16, 2025 15:48

pitrou reviewed Jun 18, 2025

View reviewed changes

Ziy1-Tan force-pushed the string_builder branch 2 times, most recently from 340254b to 9ec056e Compare June 22, 2025 03:30

raulcd approved these changes Jun 25, 2025

View reviewed changes

raulcd requested a review from pitrou June 25, 2025 08:06

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jun 25, 2025

Ziy1-Tan added 2 commits July 1, 2025 10:21

Rename arrow::util::StringBuilder and move to internal namespace

45c1cce

Signed-off-by: Ziy1-Tan <[email protected]>

Fix tests

b9c5e4d

Signed-off-by: Ziy1-Tan <[email protected]>

pitrou force-pushed the string_builder branch from 9ec056e to b9c5e4d Compare July 1, 2025 08:21

pitrou approved these changes Jul 1, 2025

View reviewed changes

pitrou merged commit 1202d79 into apache:main Jul 1, 2025
38 checks passed

pitrou removed the awaiting merge Awaiting merge label Jul 1, 2025

pitrou mentioned this pull request Jul 1, 2025

[C++] Rename arrow::util::StringBuilder and move to internal namespace #46207

Closed

This was referenced Jul 4, 2025

[CI][C++] test-r-rstudio-r-base-4.1-opensuse155 is failing #46989

Closed

GH-46989: [CI][R] Use Ubuntu 20.04 instead of OpenSUSE for R 4.1 #46990

Merged

amoeba mentioned this pull request Jul 8, 2025

[C++][Docs] Increase minimum gcc version required for building Arrow C++ from 7.1 to 9 #47025

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

GH-46207: [C++] Rename arrow::util::StringBuilder and move to internal namespace #46813

GH-46207: [C++] Rename arrow::util::StringBuilder and move to internal namespace #46813

Uh oh!

Ziy1-Tan commented Jun 14, 2025 •

edited by github-actions bot

Loading

Uh oh!

raulcd left a comment

Uh oh!

Ziy1-Tan commented Jun 16, 2025

Uh oh!

pitrou left a comment

Uh oh!

pitrou Jun 18, 2025

Uh oh!

raulcd left a comment

Uh oh!

pitrou commented Jul 1, 2025

Uh oh!

pitrou left a comment

Uh oh!

github-actions bot commented Jul 1, 2025

Uh oh!

Uh oh!

conbench-apache-arrow bot commented Jul 1, 2025

Uh oh!

kou commented Jul 4, 2025

Uh oh!

github-actions bot commented Jul 4, 2025

Uh oh!

Uh oh!

GH-46207: [C++] Rename arrow::util::StringBuilder and move to internal namespace #46813

GH-46207: [C++] Rename arrow::util::StringBuilder and move to internal namespace #46813

Uh oh!

Conversation

Ziy1-Tan commented Jun 14, 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!

raulcd left a comment

Choose a reason for hiding this comment

Uh oh!

Ziy1-Tan commented Jun 16, 2025

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

raulcd left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou commented Jul 1, 2025

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 1, 2025

Uh oh!

Uh oh!

conbench-apache-arrow bot commented Jul 1, 2025

Uh oh!

kou commented Jul 4, 2025

Uh oh!

github-actions bot commented Jul 4, 2025

Uh oh!

Uh oh!

Ziy1-Tan commented Jun 14, 2025 •

edited by github-actions bot

Loading