Skip to content

GH-50859: [C++][Parquet] Move JsonWriter to simdjson utilities - #50990

Open
rok wants to merge 5 commits into
apache:mainfrom
rok:gh-50859-parquet-without-json
Open

GH-50859: [C++][Parquet] Move JsonWriter to simdjson utilities#50990
rok wants to merge 5 commits into
apache:mainfrom
rok:gh-50859-parquet-without-json

Conversation

@rok

@rok rok commented Aug 25, 2026

Copy link
Copy Markdown
Member

Rationale for this change

Parquet uses JsonWriter when ARROW_JSON=OFF, but its implementation was only built with Arrow JSON, causing link failures.

What changes are included in this PR?

Move JsonWriter to the simdjson utilities and update its callers and CMake/Meson builds.

Are these changes tested?

Yes. CMake shared/static and Meson Parquet builds pass with JSON disabled. Unit tests and pre-commit checks also pass.

Are there any user-facing changes?

No. This only fixes the affected build configuration.

AI disclosure - this was AI generated to test alternative approach to #50900.

@pitrou

pitrou commented Aug 25, 2026

Copy link
Copy Markdown
Member

The CI failures need fixing.

@rok

rok commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

@pitrou done.

@Reranko05 Reranko05 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rok Since simdjson_internal.cc now exists, shouldn't all the non-template functions currently in simdjson_internal.h be moved into simdjson_internal.cc, leaving only declarations in the header?

@rok

rok commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

@rok Since simdjson_internal.cc now exists, shouldn't all the non-template functions currently in simdjson_internal.h be moved into simdjson_internal.cc, leaving only declarations in the header?

Done

@Reranko05 Reranko05 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rok

rok commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

Again, this PR is purely AI generated. I will mark as ready for review once I review myself.

@pitrou

pitrou commented Aug 27, 2026

Copy link
Copy Markdown
Member

@rok Are you willing to prioritize this?

@rok

rok commented Aug 27, 2026

Copy link
Copy Markdown
Member Author

@pitrou I'll review this in about an hour and ping again

@tadeja

tadeja commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

@github-actions crossbow submit example-cpp-tutorial

@github-actions

Copy link
Copy Markdown

Revision: 973b4d2

Submitted crossbow builds: ursacomputing/crossbow @ actions-5765e8701c

Task Status
example-cpp-tutorial GitHub Actions

Parquet uses JsonWriter independently of the Arrow JSON module. Move the writer into the simdjson utilities so it is available whenever simdjson is enabled, including ARROW_JSON=OFF builds. Preserve the writer files as renames and update CMake, Meson, callers, and tests.
@rok
rok force-pushed the gh-50859-parquet-without-json branch from 973b4d2 to 5a1f424 Compare August 27, 2026 14:51
Comment thread cpp/src/arrow/util/CMakeLists.txt Outdated
SOURCES
json_writer_internal_test.cc
EXTRA_LINK_LIBS
simdjson::simdjson)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use arrow::simdjson here instead? Both should work, but arrow alieas would hide the vendored/system simdjson.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if(SIMDJSON_VENDORED)
  add_library(arrow::simdjson ALIAS simdjson)
else()
  add_library(arrow::simdjson ALIAS simdjson::simdjson)
endif()

It seems better to use arrow::simdjson based on the above statements? cc @kou

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, changed to arrow::simdjson.

@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Aug 27, 2026
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 27, 2026
@rok

rok commented Aug 27, 2026

Copy link
Copy Markdown
Member Author

@github-actions crossbow submit example-cpp-tutorial

@github-actions

Copy link
Copy Markdown

Revision: 86e85bd

Submitted crossbow builds: ursacomputing/crossbow @ actions-e4578321bd

Task Status
example-cpp-tutorial GitHub Actions

@rok
rok marked this pull request as ready for review August 27, 2026 15:50
@rok
rok requested review from lidavidm, pitrou and wgtmac as code owners August 27, 2026 15:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 29 out of 30 changed files in this pull request and generated no new comments.

…ties

Fully qualify simdjson utility calls so CMake unity builds do not resolve internal to arrow::json::internal.
Copilot AI review requested due to automatic review settings August 28, 2026 10:51
@rok
rok force-pushed the gh-50859-parquet-without-json branch from bdee2b8 to ab2c41a Compare August 28, 2026 10:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 29 out of 30 changed files in this pull request and generated no new comments.

@rok

rok commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

@github-actions crossbow submit test-ubuntu-24.04-cpp-minimal-with-formats test-ubuntu-24.04-cpp-gcc-13-bundled test-ubuntu-24.04-cpp example-cpp-tutorial

@github-actions

Copy link
Copy Markdown

Revision: ab2c41a

Submitted crossbow builds: ursacomputing/crossbow @ actions-af7350541b

Task Status
example-cpp-tutorial GitHub Actions
test-ubuntu-24.04-cpp GitHub Actions
test-ubuntu-24.04-cpp-gcc-13-bundled GitHub Actions
test-ubuntu-24.04-cpp-minimal-with-formats GitHub Actions

@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Aug 28, 2026
@rok
rok requested a lite review from Copilot August 28, 2026 12:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 29 out of 30 changed files in this pull request and generated no new comments.

@rok
rok requested a balanced review from Copilot August 28, 2026 13:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 29 out of 30 changed files in this pull request and generated no new comments.

@rok

rok commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

@pitrou This is ready for review. CI failures appear unrelated and are present on main (JNI issue).

@rok
rok requested a review from wgtmac August 28, 2026 20:12
@pitrou

pitrou commented Aug 31, 2026

Copy link
Copy Markdown
Member

@github-actions crossbow submit -g cpp


Status JsonWriter::WriteValue(sj::value value) {
return internal::VisitJsonValue(
return ::arrow::internal::VisitJsonValue(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need to add this prefix.

@github-actions

Copy link
Copy Markdown

Revision: ab2c41a

Submitted crossbow builds: ursacomputing/crossbow @ actions-f7784ce147

Task Status
example-cpp-minimal-build-static GitHub Actions
example-cpp-minimal-build-static-system-dependency GitHub Actions
example-cpp-tutorial GitHub Actions
test-build-cpp-fuzz GitHub Actions
test-conda-cpp GitHub Actions
test-conda-cpp-valgrind GitHub Actions
test-debian-13-cpp-amd64 GitHub Actions
test-debian-13-cpp-i386 GitHub Actions
test-debian-experimental-cpp-gcc-15 GitHub Actions
test-fedora-42-cpp GitHub Actions
test-ubuntu-22.04-cpp GitHub Actions
test-ubuntu-22.04-cpp-bundled GitHub Actions
test-ubuntu-22.04-cpp-emscripten GitHub Actions
test-ubuntu-22.04-cpp-no-threading GitHub Actions
test-ubuntu-24.04-cpp GitHub Actions
test-ubuntu-24.04-cpp-bundled-offline GitHub Actions
test-ubuntu-24.04-cpp-gcc-13-bundled GitHub Actions
test-ubuntu-24.04-cpp-gcc-14 GitHub Actions
test-ubuntu-24.04-cpp-minimal-with-formats GitHub Actions
test-ubuntu-24.04-cpp-thread-sanitizer GitHub Actions

@pitrou

pitrou commented Aug 31, 2026

Copy link
Copy Markdown
Member

Why not put everything in simdjson_internal.h, under the same namespace?

return "unknown";
}
}
ARROW_EXPORT const char* JsonTypeName(simdjson::dom::element_type type);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, thanks for taking care of this :)

Comment thread cpp/src/arrow/CMakeLists.txt Outdated
Comment on lines +649 to +650
ARROW_UTIL_SRCS
json/object_parser.cc

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If object_parser.cc is necessary even with ARROW_JSON disabled, perhaps it should be moved outside of the json directory? It seems it's Parquet-only by the way.

Copilot AI review requested due to automatic review settings August 31, 2026 11:57
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants