GH-50627: [C++] Migrate from_string.cc to simdjson by taepper · Pull Request #50653 · apache/arrow

taepper · 2026-07-27T08:48:33Z

Rationale for this change

This is part of the work for #35460. Back in January I started working on this change-set, but after discussions came up regarding the use of the simdjson::dom vs simdjson::ondemand front-end the development went stale. I saw that @Reranko05 started the work on #35460 and already made great progress, well done!

I still wanted to put up this change-set as a possible view-point, but feel free to disregard @Reranko05 if you already started your own work.

What changes are included in this PR?

This changes from_string.cc to use simdjson instead of RapidJSON.

Are there any user-facing changes?

Yes, the API has changed to no longer allow non-utf8 compatible strings. simdjson does not support non-compliant inputs and does not intent to support it in the future [1] [2] [3]

All call-sites in tests only used a transformation to non-utf8 json as an intermediate result to easily go from string -> Array, so I changed these to use different helper methods instead.

Another difficult problem arises in the handling of NaN and Inf literals in inputs. simdjson follows the json standard, whereas RapidJSON implemented an extension where the following is a valid input:

{"x": Inf, "y": -Inf, "z": NaN, "n": Infinity, "m": -Infinity}

We need to decide whether to be breaking here or not. This PR now contains a non-breaking version of the change. This can be quite elegantly added by using the public ondemand api's function to retrieve the raw token string in the ConvertNumber function.

Alternatively, we can wait for the next simdjson major release, which will ship with support for a SIMDJSON_ENABLE_NAN_INF compilation flag as per this pull request. (this was only merged to simdjson main)

Are these changes tested?

Yes

GitHub Issue: [C++] Migrate to simdjson in from_string.cc and tests #50627

Reranko05 · 2026-07-27T08:54:47Z

Thanks for working on this! This part of the migration (from_string.cc) was planned for a later PR in my migration series, so I hadn't reached it yet. I appreciate you picking it up :)

taepper · 2026-07-27T12:49:18Z

Putting

template <typename BuilderType>
std::shared_ptr<Array> BinaryArrayFromStrings(
    const std::shared_ptr<DataType>& type,
    const std::vector<std::optional<std::string>>& values) {
  std::unique_ptr<ArrayBuilder> builder;
  ARROW_CHECK_OK(MakeBuilder(default_memory_pool(), type, &builder));
  auto& concrete_builder = checked_cast<BuilderType&>(*builder);
  for (const auto& value : values) {
    if (value.has_value()) {
      ARROW_CHECK_OK(concrete_builder.Append(*value));
    } else {
      ARROW_CHECK_OK(concrete_builder.AppendNull());
    }
  }
  std::shared_ptr<Array> array;
  ARROW_CHECK_OK(concrete_builder.Finish(&array));
  return array;
}

into gdb.cc felt a little bit ugly, but as the file only contains tests and the helper method is quite short it might be fine? I did not see a good other place, as changing the library interface for a test-helper-method seems excessive, and including from .../testing/... in gdb.cc would break boundaries..

pitrou · 2026-07-27T13:06:13Z

into gdb.cc felt a little bit ugly, but as the file only contains tests and the helper method is quite short it might be fine?

Yes, I think it's fine.

pitrou · 2026-07-28T07:06:16Z

+}
+
+template <typename SimdjsonValueType>
+Result<SimdjsonValueType> GetAs(sj::value& value) {


@Reranko05 FYI

taepper · 2026-07-28T07:10:21Z

+    return "string";
+  } else if constexpr (std::is_same_v<SimdjsonClass, bool>) {
+    return "boolean";
+  } else if constexpr (std::is_same_v<SimdjsonClass, std::monostate>) {


std::monostate

We probably want to add a typedef for this (if we want to keep this at all?). Alternatively, we could check the null case also always by hand

pitrou · 2026-07-28T07:11:38Z

+  };
+
+  // Use fold expression to process all handlers in order
+  (process_one(std::forward<Handlers>(handlers)) && ...);


Note that Status supports the & operator, which might allow simplifying this a bit.

I did now use it, but am unsure about readability improvements. Is this what you meant?

It made the body a bit cleaner, but it is now more difficult to reason about the error message, but maybe that is just me ^^

Yes, that's what I had in mind. I understand what you mean with "difficult to reason about the error message", but normally the first should be kept :)

pitrou · 2026-07-28T08:31:31Z

Hmm, this CI error looks related: https://github.com/apache/arrow/actions/runs/30342146109/job/90219928526?pr=50653

(perhaps my suggestion re Status::operator& was wrong? sorry)

pitrou · 2026-07-28T08:32:37Z

+    return "string";
+  } else if constexpr (std::is_same_v<SimdjsonClass, bool>) {
+    return "boolean";
+  } else if constexpr (std::is_same_v<SimdjsonClass, std::monostate>) {


Is std::monostate somehow imposed by simdjson? Otherwise can we use something more distinctive?

Yes, I replaced it with a custom empty class

pitrou · 2026-07-28T08:33:00Z

+template <typename>
+inline constexpr bool kAlwaysFalse = false;


Doesn't seem used, which errors out on some compilers (see CI :-)).

I see. Was able to replace with static_assert(false,..) inline. Not sure why this produced an error for me before. Might had missed a constexpr in one of the elif branches and thought this indirection was necessary

I see in the CI run that the way I solved it is indeed not portable:

/Users/runner/work/arrow/arrow/cpp/src/arrow/json/from_string.cc:109:5: error: static assertion failed: unmapped simdjson value type static_assert(false, "unmapped simdjson value type"); ^ ~~~~~

taepper · 2026-07-28T09:02:29Z

Hmm, this CI error looks related: https://github.com/apache/arrow/actions/runs/30342146109/job/90219928526?pr=50653

(perhaps my suggestion re Status::operator& was wrong? sorry)

Yes, it is related. Nono, the mistake was on my part. But yes, consuming more elements from the iterator after an error is UB in simdjson. Sloppy by me, sorry. I refactored it and found a much cleaner solution to only template the expected_size to the function. Then we do not need to fold and can just have a loop. (Also, we can use RETURN_NOT_OK)

migrate from_string.cc to simdjson

c526a91

taepper requested review from pitrou and wgtmac as code owners July 27, 2026 08:48

github-actions Bot added the awaiting review Awaiting review label Jul 27, 2026

pitrou reviewed Jul 27, 2026

View reviewed changes

Comment thread cpp/src/arrow/json/from_string.cc Outdated

Comment thread cpp/src/arrow/json/from_string.cc Outdated

Comment thread cpp/src/arrow/json/from_string.cc Outdated

Comment thread cpp/src/arrow/json/from_string.cc Outdated

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 27, 2026

fix review comments

2f9559a

github-actions Bot added Component: Parquet Component: C++ Component: Python labels Jul 27, 2026

taepper force-pushed the GH-35460 branch from df45e8a to 6face6e Compare July 27, 2026 11:58

also add helper method for non-utf8 inputs to python/gdb.cc

6b76b3d

taepper force-pushed the GH-35460 branch from 6face6e to 6b76b3d Compare July 27, 2026 12:08

taepper requested a review from pitrou July 27, 2026 12:46

pitrou reviewed Jul 27, 2026

View reviewed changes

Comment thread cpp/src/arrow/json/from_string.cc Outdated

add helpers to fit simdjson into arrow results

8627a6f

pitrou reviewed Jul 28, 2026

View reviewed changes

taepper commented Jul 28, 2026

View reviewed changes

pitrou reviewed Jul 28, 2026

View reviewed changes

taepper added 2 commits July 28, 2026 10:20

use Status::operator& and linter error

16ddada

improve error context

fe4158f

pitrou reviewed Jul 28, 2026

View reviewed changes

cleaner function names

caeb78f

refactor ProcessJsonArrayElements

d318388

		template <typename>
		inline constexpr bool kAlwaysFalse = false;

Uh oh!

Conversation

taepper commented Jul 27, 2026 • 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 there any user-facing changes?

Are these changes tested?

Uh oh!

Reranko05 commented Jul 27, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taepper commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pitrou commented Jul 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

pitrou commented Jul 28, 2026

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!

taepper Jul 28, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

taepper commented Jul 28, 2026 • 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

taepper commented Jul 27, 2026 •

edited by github-actions Bot

Loading

taepper commented Jul 27, 2026 •

edited

Loading

taepper Jul 28, 2026 •

edited

Loading

taepper commented Jul 28, 2026 •

edited

Loading