From c526a91c0c3d9eccc5b795564060a7e53de59a17 Mon Sep 17 00:00:00 2001 From: Alexander Taepper Date: Wed, 28 Jan 2026 13:45:32 +0100 Subject: [PATCH 01/10] migrate `from_string.cc` to `simdjson` --- .../arrow/compute/kernels/scalar_cast_test.cc | 23 +- .../compute/kernels/scalar_string_test.cc | 81 +-- cpp/src/arrow/extension/json_test.cc | 4 +- cpp/src/arrow/json/from_string.cc | 629 +++++++++++------- cpp/src/arrow/json/from_string_test.cc | 18 +- cpp/src/arrow/scalar_test.cc | 7 +- cpp/src/arrow/testing/builder.h | 34 + .../parquet/arrow/arrow_reader_writer_test.cc | 27 +- 8 files changed, 519 insertions(+), 304 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_cast_test.cc b/cpp/src/arrow/compute/kernels/scalar_cast_test.cc index 51e6ca534cd5..364a4bd436b6 100644 --- a/cpp/src/arrow/compute/kernels/scalar_cast_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_cast_test.cc @@ -31,6 +31,7 @@ #include "arrow/chunked_array.h" #include "arrow/extension_type.h" #include "arrow/status.h" +#include "arrow/testing/builder.h" #include "arrow/testing/extension_type.h" #include "arrow/testing/gtest_util.h" #include "arrow/testing/random.h" @@ -56,16 +57,7 @@ using internal::checked_pointer_cast; namespace compute { static std::shared_ptr InvalidUtf8(std::shared_ptr type) { - return ArrayFromJSON(type, - "[" - R"( - "Hi", - "olá mundo", - "你好世界", - "", - )" - "\"\xa0\xa1\"" - "]"); + return BinaryArrayFromStrings(type, {"Hi", "olá mundo", "你好世界", "", "\xa0\xa1"}); } static std::shared_ptr FixedSizeInvalidUtf8(std::shared_ptr type) { @@ -73,16 +65,7 @@ static std::shared_ptr FixedSizeInvalidUtf8(std::shared_ptr typ // Assume a particular width for testing EXPECT_EQ(3, checked_cast(*type).byte_width()); } - return ArrayFromJSON(type, - "[" - R"( - "Hi!", - "lá", - "你", - " ", - )" - "\"\xa0\xa1\xa2\"" - "]"); + return BinaryArrayFromStrings(type, {"Hi!", "lá", "你", " ", "\xa0\xa1\xa2"}); } static std::vector> kNumericTypes = { diff --git a/cpp/src/arrow/compute/kernels/scalar_string_test.cc b/cpp/src/arrow/compute/kernels/scalar_string_test.cc index b279f991f6ea..b90cb1c80d0a 100644 --- a/cpp/src/arrow/compute/kernels/scalar_string_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_string_test.cc @@ -1102,7 +1102,7 @@ TYPED_TEST(TestStringKernels, Utf8Reverse) { // inputs with malformed utf8 chars would produce garbage output, but the end result // would produce arrays with same lengths. Hence checking offset buffer equality - auto malformed_input = ArrayFromJSON(this->type(), "[\"ɑ\xFFɑa\", \"ɽ\xe1\xbdɽa\"]"); + auto malformed_input = this->MakeArray({"ɑ\xFFɑa", "ɽ\xe1\xbdɽa"}); const Result& res = CallFunction("utf8_reverse", {malformed_input}); ASSERT_TRUE(res->array()->buffers[1]->Equals(*malformed_input->data()->buffers[1])); } @@ -1225,7 +1225,7 @@ TYPED_TEST(TestStringKernels, Utf8Upper) { this->CheckUnary("utf8_upper", "[\"ɑɑɑɑ\"]", this->type(), "[\"ⱭⱭⱭⱭ\"]"); // Test invalid data - auto invalid_input = ArrayFromJSON(this->type(), "[\"ɑa\xFFɑ\", \"ɽ\xe1\xbdɽaa\"]"); + auto invalid_input = this->MakeArray({"ɑa\xFFɑ", "ɽ\xe1\xbdɽaa"}); EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid, testing::HasSubstr("Invalid UTF8 sequence"), CallFunction("utf8_upper", {invalid_input})); } @@ -1247,7 +1247,7 @@ TYPED_TEST(TestStringKernels, Utf8Lower) { this->CheckUnary("utf8_lower", "[\"ȺȺȺȺ\"]", this->type(), "[\"ⱥⱥⱥⱥ\"]"); // Test invalid data - auto invalid_input = ArrayFromJSON(this->type(), "[\"Ⱥa\xFFⱭ\", \"Ɽ\xe1\xbdⱤaA\"]"); + auto invalid_input = this->MakeArray({"Ⱥa\xFFⱭ", "Ɽ\xe1\xbdⱤaA"}); EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid, testing::HasSubstr("Invalid UTF8 sequence"), CallFunction("utf8_lower", {invalid_input})); } @@ -1267,7 +1267,7 @@ TYPED_TEST(TestStringKernels, Utf8SwapCase) { "[\"HeLLo, wOrLD!\", \"$. a35?\"]"); // Test invalid data - auto invalid_input = ArrayFromJSON(this->type(), "[\"Ⱥa\xFFⱭ\", \"Ɽ\xe1\xbdⱤaA\"]"); + auto invalid_input = this->MakeArray({"Ⱥa\xFFⱭ", "Ɽ\xe1\xbdⱤaA"}); EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid, testing::HasSubstr("Invalid UTF8 sequence"), CallFunction("utf8_swapcase", {invalid_input})); } @@ -2516,101 +2516,92 @@ TYPED_TEST(TestBinaryKernels, SliceBytesPosPos) { SliceOptions options{2, 4}; this->CheckUnary( "binary_slice", - "[\"\", \"a\", \"ab\", \"a\xc2\xa2\", \"ab\xc2\xa2\", \"ab\xc2\xffZ\"]", - this->type(), "[\"\", \"\", \"\", \"\xa2\", \"\xc2\xa2\", \"\xc2\xff\"]", &options); + this->MakeArray({"", "a", "ab", "a\xc2\xa2", "ab\xc2\xa2", "ab\xc2\xffZ"}), + this->MakeArray({"", "", "", "\xa2", "\xc2\xa2", "\xc2\xff"}), &options); SliceOptions options_step{1, 5, 2}; this->CheckUnary( "binary_slice", - "[\"\", \"a\", \"ab\", \"a\xc2\xa2\", \"ab\xc2\xa2\", \"ab\xc2\xffZ\"]", - this->type(), "[\"\", \"\", \"b\", \"\xc2\", \"b\xa2\", \"b\xff\"]", &options_step); + this->MakeArray({"", "a", "ab", "a\xc2\xa2", "ab\xc2\xa2", "ab\xc2\xffZ"}), + this->MakeArray({"", "", "b", "\xc2", "b\xa2", "b\xff"}), &options_step); SliceOptions options_step_neg{5, 1, -2}; this->CheckUnary( "binary_slice", - "[\"\", \"a\", \"ab\", \"a\xc2\xa2\", \"ab\xc2\xa2\", \"ab\xc2\xffZ\"]", - this->type(), "[\"\", \"\", \"\", \"\xa2\", \"\xa2\", \"Z\xc2\"]", - &options_step_neg); + this->MakeArray({"", "a", "ab", "a\xc2\xa2", "ab\xc2\xa2", "ab\xc2\xffZ"}), + this->MakeArray({"", "", "", "\xa2", "\xa2", "Z\xc2"}), &options_step_neg); options_step_neg.stop = 0; this->CheckUnary( "binary_slice", - "[\"\", \"a\", \"ab\", \"a\xc2\xa2\", \"aZ\xc2\xa2\", \"ab\xc2\xffZ\"]", - this->type(), "[\"\", \"\", \"b\", \"\xa2\", \"\xa2Z\", \"Z\xc2\"]", - &options_step_neg); + this->MakeArray({"", "a", "ab", "a\xc2\xa2", "aZ\xc2\xa2", "ab\xc2\xffZ"}), + this->MakeArray({"", "", "b", "\xa2", "\xa2Z", "Z\xc2"}), &options_step_neg); } TYPED_TEST(TestBinaryKernels, SliceBytesPosNeg) { SliceOptions options{2, -1}; this->CheckUnary( "binary_slice", - "[\"\", \"a\", \"ab\", \"a\xc2\xa2\", \"aZ\xc2\xa2\", \"ab\xc2\xffZ\"]", - this->type(), "[\"\", \"\", \"\", \"\", \"\xc2\", \"\xc2\xff\"]", &options); + this->MakeArray({"", "a", "ab", "a\xc2\xa2", "aZ\xc2\xa2", "ab\xc2\xffZ"}), + this->MakeArray({"", "", "", "", "\xc2", "\xc2\xff"}), &options); SliceOptions options_step{1, -1, 2}; this->CheckUnary( "binary_slice", - "[\"\", \"a\", \"ab\", \"a\xc2\xa2\", \"aZ\xc2\xa2\", \"ab\xc2\xffZ\"]", - this->type(), "[\"\", \"\", \"\", \"\xc2\", \"Z\", \"b\xff\"]", &options_step); + this->MakeArray({"", "a", "ab", "a\xc2\xa2", "aZ\xc2\xa2", "ab\xc2\xffZ"}), + this->MakeArray({"", "", "", "\xc2", "Z", "b\xff"}), &options_step); SliceOptions options_step_neg{3, -4, -2}; this->CheckUnary( "binary_slice", - "[\"\", \"a\", \"ab\", \"Z\xc2\xa2\", \"aZ\xc2\xa2\", \"ab\xc2\xffZ\"]", - this->type(), "[\"\", \"a\", \"b\", \"\xa2Z\", \"\xa2Z\", \"\xff\"]", - &options_step_neg); + this->MakeArray({"", "a", "ab", "Z\xc2\xa2", "aZ\xc2\xa2", "ab\xc2\xffZ"}), + this->MakeArray({"", "a", "b", "\xa2Z", "\xa2Z", "\xff"}), &options_step_neg); options_step_neg.stop = -5; this->CheckUnary( "binary_slice", - "[\"\", \"a\", \"ab\", \"Z\xc2\xa2\", \"aZ\xc2\xa2\", \"aP\xc2\xffZ\"]", - this->type(), "[\"\", \"a\", \"b\", \"\xa2Z\", \"\xa2Z\", \"\xffP\"]", - &options_step_neg); + this->MakeArray({"", "a", "ab", "Z\xc2\xa2", "aZ\xc2\xa2", "aP\xc2\xffZ"}), + this->MakeArray({"", "a", "b", "\xa2Z", "\xa2Z", "\xffP"}), &options_step_neg); } TYPED_TEST(TestBinaryKernels, SliceBytesNegNeg) { SliceOptions options{-2, -1}; this->CheckUnary( "binary_slice", - "[\"\", \"a\", \"ab\", \"Z\xc2\xa2\", \"aZ\xc2\xa2\", \"ab\xc2\xffZ\"]", - this->type(), "[\"\", \"\", \"a\", \"\xc2\", \"\xc2\", \"\xff\"]", &options); + this->MakeArray({"", "a", "ab", "Z\xc2\xa2", "aZ\xc2\xa2", "ab\xc2\xffZ"}), + this->MakeArray({"", "", "a", "\xc2", "\xc2", "\xff"}), &options); SliceOptions options_step{-4, -1, 2}; this->CheckUnary( "binary_slice", - "[\"\", \"a\", \"ab\", \"Z\xc2\xa2\", \"aZ\xc2\xa2\", \"aP\xc2\xffZ\"]", - this->type(), "[\"\", \"\", \"a\", \"Z\", \"a\xc2\", \"P\xff\"]", &options_step); + this->MakeArray({"", "a", "ab", "Z\xc2\xa2", "aZ\xc2\xa2", "aP\xc2\xffZ"}), + this->MakeArray({"", "", "a", "Z", "a\xc2", "P\xff"}), &options_step); SliceOptions options_step_neg{-1, -3, -2}; this->CheckUnary( "binary_slice", - "[\"\", \"a\", \"ab\", \"Z\xc2\xa2\", \"aZ\xc2\xa2\", \"aP\xc2\xffZ\"]", - this->type(), "[\"\", \"a\", \"b\", \"\xa2\", \"\xa2\", \"Z\"]", &options_step_neg); + this->MakeArray({"", "a", "ab", "Z\xc2\xa2", "aZ\xc2\xa2", "aP\xc2\xffZ"}), + this->MakeArray({"", "a", "b", "\xa2", "\xa2", "Z"}), &options_step_neg); options_step_neg.stop = -4; this->CheckUnary( "binary_slice", - "[\"\", \"a\", \"ab\", \"Z\xc2\xa2\", \"aZ\xc2\xa2\", \"aP\xc2\xffZ\"]", - this->type(), "[\"\", \"a\", \"b\", \"\xa2Z\", \"\xa2Z\", \"Z\xc2\"]", - &options_step_neg); + this->MakeArray({"", "a", "ab", "Z\xc2\xa2", "aZ\xc2\xa2", "aP\xc2\xffZ"}), + this->MakeArray({"", "a", "b", "\xa2Z", "\xa2Z", "Z\xc2"}), &options_step_neg); } TYPED_TEST(TestBinaryKernels, SliceBytesNegPos) { SliceOptions options{-2, 4}; this->CheckUnary( "binary_slice", - "[\"\", \"a\", \"ab\", \"Z\xc2\xa2\", \"aZ\xc2\xa2\", \"aP\xc2\xffZ\"]", - this->type(), "[\"\", \"a\", \"ab\", \"\xc2\xa2\", \"\xc2\xa2\", \"\xff\"]", - &options); + this->MakeArray({"", "a", "ab", "Z\xc2\xa2", "aZ\xc2\xa2", "aP\xc2\xffZ"}), + this->MakeArray({"", "a", "ab", "\xc2\xa2", "\xc2\xa2", "\xff"}), &options); SliceOptions options_step{-4, 4, 2}; this->CheckUnary( "binary_slice", - "[\"\", \"a\", \"ab\", \"Z\xc2\xa2\", \"aZ\xc2\xa2\", \"aP\xc2\xffZ\"]", - this->type(), "[\"\", \"a\", \"a\", \"Z\xa2\", \"a\xc2\", \"P\xff\"]", - &options_step); + this->MakeArray({"", "a", "ab", "Z\xc2\xa2", "aZ\xc2\xa2", "aP\xc2\xffZ"}), + this->MakeArray({"", "a", "a", "Z\xa2", "a\xc2", "P\xff"}), &options_step); SliceOptions options_step_neg{-1, 1, -2}; this->CheckUnary( "binary_slice", - "[\"\", \"a\", \"ab\", \"Z\xc2\xa2\", \"aZ\xc2\xa2\", \"aP\xc2\xffZ\"]", - this->type(), "[\"\", \"\", \"\", \"\xa2\", \"\xa2\", \"Z\xc2\"]", - &options_step_neg); + this->MakeArray({"", "a", "ab", "Z\xc2\xa2", "aZ\xc2\xa2", "aP\xc2\xffZ"}), + this->MakeArray({"", "", "", "\xa2", "\xa2", "Z\xc2"}), &options_step_neg); options_step_neg.stop = 0; this->CheckUnary( "binary_slice", - "[\"\", \"a\", \"ab\", \"Z\xc2\xa2\", \"aZ\xc2\xa2\", \"aP\xc2\xffZ\"]", - this->type(), "[\"\", \"\", \"b\", \"\xa2\", \"\xa2Z\", \"Z\xc2\"]", - &options_step_neg); + this->MakeArray({"", "a", "ab", "Z\xc2\xa2", "aZ\xc2\xa2", "aP\xc2\xffZ"}), + this->MakeArray({"", "", "b", "\xa2", "\xa2Z", "Z\xc2"}), &options_step_neg); } TYPED_TEST(TestStringKernels, PadAscii) { diff --git a/cpp/src/arrow/extension/json_test.cc b/cpp/src/arrow/extension/json_test.cc index b938ddb2cfef..67164def5318 100644 --- a/cpp/src/arrow/extension/json_test.cc +++ b/cpp/src/arrow/extension/json_test.cc @@ -20,6 +20,7 @@ #include "arrow/array/validate.h" #include "arrow/ipc/test_common.h" #include "arrow/record_batch.h" +#include "arrow/testing/builder.h" #include "arrow/testing/gtest_util.h" #include "parquet/exception.h" @@ -62,9 +63,10 @@ TEST_F(TestJsonExtensionType, JsonRoundtrip) { } TEST_F(TestJsonExtensionType, InvalidUTF8) { + const std::vector invalid_values = {"Ⱥa\xFFⱭ", "Ɽ\xe1\xbdⱤaA"}; for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) { auto json_type = json(storage_type); - auto invalid_input = ArrayFromJSON(storage_type, "[\"Ⱥa\xFFⱭ\", \"Ɽ\xe1\xbdⱤaA\"]"); + auto invalid_input = BinaryArrayFromStrings(storage_type, invalid_values); auto ext_arr = ExtensionType::WrapArray(json_type, invalid_input); ASSERT_RAISES_WITH_MESSAGE(Invalid, diff --git a/cpp/src/arrow/json/from_string.cc b/cpp/src/arrow/json/from_string.cc index e35a362f5a2e..42f8a72ad6e2 100644 --- a/cpp/src/arrow/json/from_string.cc +++ b/cpp/src/arrow/json/from_string.cc @@ -15,10 +15,14 @@ // specific language governing permissions and limitations // under the License. +#include +#include #include #include +#include #include #include +#include #include #include @@ -32,23 +36,21 @@ #include "arrow/array/builder_union.h" #include "arrow/chunked_array.h" #include "arrow/json/from_string.h" +#include "arrow/result.h" #include "arrow/scalar.h" +#include "arrow/status.h" +#include "arrow/type.h" #include "arrow/type_traits.h" #include "arrow/util/checked_cast.h" #include "arrow/util/decimal.h" #include "arrow/util/float16.h" #include "arrow/util/logging_internal.h" +#include "arrow/util/unreachable.h" #include "arrow/util/value_parsing.h" -#include "arrow/json/rapidjson_defs.h" +#include -#include -#include -#include -#include -#include - -namespace rj = arrow::rapidjson; +namespace sj = simdjson::ondemand; namespace arrow { @@ -62,30 +64,32 @@ using ::arrow::internal::checked_pointer_cast; namespace { -constexpr auto kParseFlags = rj::kParseFullPrecisionFlag | rj::kParseNanAndInfFlag; - -const char* JsonTypeName(rj::Type json_type) { - switch (json_type) { - case rapidjson::kNullType: - return "null"; - case rapidjson::kFalseType: - return "false"; - case rapidjson::kTrueType: - return "true"; - case rapidjson::kObjectType: - return "object"; - case rapidjson::kArrayType: +const char* JsonTypeName(sj::json_type type) { + switch (type) { + case sj::json_type::array: return "array"; - case rapidjson::kStringType: - return "string"; - case rapidjson::kNumberType: + case sj::json_type::object: + return "object"; + case sj::json_type::number: return "number"; + case sj::json_type::string: + return "string"; + case sj::json_type::boolean: + return "boolean"; + case sj::json_type::null: + return "null"; default: return "unknown"; } } -Status JSONTypeError(const char* expected_type, rj::Type json_type) { +Status JSONTypeError(const char* expected_type, + simdjson::simdjson_result json_type_result) { + sj::json_type json_type; + if (std::move(json_type_result).get(json_type) != simdjson::SUCCESS) { + return Status::Invalid("Expected ", expected_type, + " or null, got malformed JSON value"); + } return Status::Invalid("Expected ", expected_type, " or null, got JSON type ", JsonTypeName(json_type)); } @@ -96,11 +100,17 @@ class JSONConverter { virtual Status Init() { return Status::OK(); } - virtual Status AppendValue(const rj::Value& json_obj) = 0; + virtual Status AppendValue(sj::value& json_obj) = 0; Status AppendNull() { return this->builder()->AppendNull(); } - virtual Status AppendValues(const rj::Value& json_array) = 0; + /// Append all elements of `json_array` to this converter's builder and + /// return the number of elements that were appended. The count lets callers + /// with a size expectation (e.g. fixed-size lists or single scalars) + /// validate it without a second pass over the forward-only JSON input. Note + /// that it only counts the elements this call consumed; callers parsing a + /// whole document must check `document::at_end()` to detect trailing input. + virtual Result AppendValues(sj::array& json_array) = 0; virtual std::shared_ptr builder() = 0; @@ -124,20 +134,18 @@ Status GetConverter(const std::shared_ptr&, template class ConcreteConverter : public JSONConverter { public: - Result SizeOfJSONArray(const rj::Value& json_obj) { - if (!json_obj.IsArray()) { - return JSONTypeError("array", json_obj.GetType()); - } - return json_obj.Size(); - } - - Status AppendValues(const rj::Value& json_array) final { + Result AppendValues(sj::array& json_array) final { auto self = static_cast(this); - ARROW_ASSIGN_OR_RAISE(auto size, SizeOfJSONArray(json_array)); - for (uint32_t i = 0; i < size; ++i) { - RETURN_NOT_OK(self->AppendValue(json_array[i])); + int32_t num_elements = 0; + for (auto element : json_array) { + sj::value value; + if (element.get(value) != simdjson::SUCCESS) { + return Status::Invalid("Could not iterate elements of JSON array"); + } + RETURN_NOT_OK(self->AppendValue(value)); + num_elements++; } - return Status::OK(); + return num_elements; } const std::shared_ptr& value_type() { @@ -167,11 +175,11 @@ class NullConverter final : public ConcreteConverter { builder_ = std::make_shared(); } - Status AppendValue(const rj::Value& json_obj) override { - if (json_obj.IsNull()) { + Status AppendValue(sj::value& json_obj) override { + if (json_obj.is_null()) { return AppendNull(); } - return JSONTypeError("null", json_obj.GetType()); + return JSONTypeError("null", json_obj.type()); } std::shared_ptr builder() override { return builder_; } @@ -190,17 +198,19 @@ class BooleanConverter final : public ConcreteConverter { builder_ = std::make_shared(); } - Status AppendValue(const rj::Value& json_obj) override { - if (json_obj.IsNull()) { + Status AppendValue(sj::value& json_obj) override { + if (json_obj.is_null()) { return AppendNull(); } - if (json_obj.IsBool()) { - return builder_->Append(json_obj.GetBool()); + bool bool_value; + if (json_obj.get(bool_value) == simdjson::SUCCESS) { + return builder_->Append(bool_value); } - if (json_obj.IsInt()) { - return builder_->Append(json_obj.GetInt() != 0); + int64_t int_value; + if (json_obj.get(int_value) == simdjson::SUCCESS) { + return builder_->Append(int_value != 0); } - return JSONTypeError("boolean", json_obj.GetType()); + return JSONTypeError("boolean", json_obj.type()); } std::shared_ptr builder() override { return builder_; } @@ -214,11 +224,11 @@ class BooleanConverter final : public ConcreteConverter { // Convert single signed integer value (also {Date,Time}{32,64} and Timestamp) template -enable_if_physical_signed_integer ConvertNumber(const rj::Value& json_obj, +enable_if_physical_signed_integer ConvertNumber(sj::value& json_obj, const DataType& type, typename T::c_type* out) { - if (json_obj.IsInt64()) { - int64_t v64 = json_obj.GetInt64(); + int64_t v64; + if (json_obj.get(v64) == simdjson::SUCCESS) { *out = static_cast(v64); if (*out == v64) { return Status::OK(); @@ -227,17 +237,17 @@ enable_if_physical_signed_integer ConvertNumber(const rj::Value& json } } else { *out = static_cast(0); - return JSONTypeError("signed int", json_obj.GetType()); + return JSONTypeError("int", json_obj.type()); } } // Convert single unsigned integer value template -enable_if_unsigned_integer ConvertNumber(const rj::Value& json_obj, +enable_if_unsigned_integer ConvertNumber(sj::value& json_obj, const DataType& type, typename T::c_type* out) { - if (json_obj.IsUint64()) { - uint64_t v64 = json_obj.GetUint64(); + uint64_t v64; + if (json_obj.get(v64) == simdjson::SUCCESS) { *out = static_cast(v64); if (*out == v64) { return Status::OK(); @@ -246,46 +256,119 @@ enable_if_unsigned_integer ConvertNumber(const rj::Value& json_obj, } } else { *out = static_cast(0); - return JSONTypeError("unsigned int", json_obj.GetType()); + return JSONTypeError("unsigned int", json_obj.type()); + } +} + +// Match the std::string_view against NaN, Inf, Infinity with optional leading minus +bool NonFiniteDoubleFromString(std::string_view str, double* out) { + const bool negative = !str.empty() && str.front() == '-'; + if (negative) { + str.remove_prefix(1); + } + if (str == "NaN") { + *out = std::numeric_limits::quiet_NaN(); + } else if (str == "Inf" || str == "Infinity") { + *out = std::numeric_limits::infinity(); + } else { + return false; + } + if (negative) { + *out = -*out; + } + return true; +} + +bool NonFiniteDoubleFromRawToken(sj::value& json_obj, double* out) { + std::string_view token = json_obj.raw_json_token(); + // The raw token includes any trailing whitespace up to the next token + while (!token.empty() && std::isspace(static_cast(token.back()))) { + token.remove_suffix(1); } + return NonFiniteDoubleFromString(token, out); +} + +// Get a double from a JSON value that is either a number, a bare +// NaN/Inf/-Inf/Infinity/-Infinity token +bool DoubleFromJsonValue(sj::value& json_obj, double* out) { + if (json_obj.get(*out) == simdjson::SUCCESS) { + return true; + } + return NonFiniteDoubleFromRawToken(json_obj, out); } // Convert float16/HalfFloatType template -enable_if_half_float ConvertNumber(const rj::Value& json_obj, - const DataType& type, uint16_t* out) { - if (json_obj.IsDouble()) { - double f64 = json_obj.GetDouble(); - *out = Float16(f64).bits(); - return Status::OK(); - } else if (json_obj.IsUint()) { - uint32_t u32t = json_obj.GetUint(); - double f64 = static_cast(u32t); - *out = Float16(f64).bits(); - return Status::OK(); - } else if (json_obj.IsInt()) { - int32_t i32t = json_obj.GetInt(); - double f64 = static_cast(i32t); +enable_if_half_float ConvertNumber(sj::value& json_obj, const DataType& type, + uint16_t* out) { + double f64; + if (DoubleFromJsonValue(json_obj, &f64)) { *out = Float16(f64).bits(); return Status::OK(); - } else { - *out = static_cast(0); - return JSONTypeError("unsigned int", json_obj.GetType()); } + *out = static_cast(0); + return JSONTypeError("number", json_obj.type()); } // Convert single floating point value template -enable_if_physical_floating_point ConvertNumber(const rj::Value& json_obj, +enable_if_physical_floating_point ConvertNumber(sj::value& json_obj, const DataType& type, typename T::c_type* out) { - if (json_obj.IsNumber()) { - *out = static_cast(json_obj.GetDouble()); + double f64; + if (DoubleFromJsonValue(json_obj, &f64)) { + *out = static_cast(f64); return Status::OK(); - } else { - *out = static_cast(0); - return JSONTypeError("number", json_obj.GetType()); } + *out = static_cast(0); + return JSONTypeError("number", json_obj.type()); +} + +// ------------------------------------------------------------------------ +// Helper to process a JSON array with exactly N elements, calling a handler for each. +// Each handler is a callable taking sj::value& and returning Status. +template +Status ProcessJsonArrayElements(sj::array& json_array, const char* error_context, + Handlers&&... handlers) { + constexpr size_t expected_size = sizeof...(Handlers); + auto it = json_array.begin(); + auto end = json_array.end(); + + size_t index = 0; + Status result = Status::OK(); + + auto process_one = [&](auto&& handler) -> bool { + if (!result.ok()) return false; + + if (it == end) { + result = Status::Invalid(error_context, " must have exactly ", expected_size, + " elements, had ", index); + return false; + } + + sj::value element; + auto error = (*it).get(element); + if (error) { + result = Status::Invalid("Failed to get element ", index, " from ", error_context); + return false; + } + + result = handler(element); + ++it; + ++index; + return result.ok(); + }; + + // Use fold expression to process all handlers in order + (process_one(std::forward(handlers)) && ...); + + if (!result.ok()) return result; + + if (it != end) { + return Status::Invalid(error_context, " must have exactly ", expected_size, + " elements, had more"); + } + return Status::OK(); } // ------------------------------------------------------------------------ @@ -303,8 +386,8 @@ class IntegerConverter final Status Init() override { return this->MakeConcreteBuilder(&builder_); } - Status AppendValue(const rj::Value& json_obj) override { - if (json_obj.IsNull()) { + Status AppendValue(sj::value& json_obj) override { + if (json_obj.is_null()) { return this->AppendNull(); } c_type value; @@ -330,8 +413,8 @@ class FloatConverter final : public ConcreteConverterMakeConcreteBuilder(&builder_); } - Status AppendValue(const rj::Value& json_obj) override { - if (json_obj.IsNull()) { + Status AppendValue(sj::value& json_obj) override { + if (json_obj.is_null()) { return this->AppendNull(); } c_type value; @@ -360,22 +443,22 @@ class DecimalConverter final Status Init() override { return this->MakeConcreteBuilder(&builder_); } - Status AppendValue(const rj::Value& json_obj) override { - if (json_obj.IsNull()) { + Status AppendValue(sj::value& json_obj) override { + if (json_obj.is_null()) { return this->AppendNull(); } - if (json_obj.IsString()) { + std::string_view string_value; + if (json_obj.get(string_value) == simdjson::SUCCESS) { int32_t precision, scale; DecimalValue d; - auto view = std::string_view(json_obj.GetString(), json_obj.GetStringLength()); - RETURN_NOT_OK(DecimalValue::FromString(view, &d, &precision, &scale)); + RETURN_NOT_OK(DecimalValue::FromString(string_value, &d, &precision, &scale)); if (scale != decimal_type_->scale()) { return Status::Invalid("Invalid scale for decimal: expected ", decimal_type_->scale(), ", got ", scale); } return builder_->Append(d); } - return JSONTypeError("decimal string", json_obj.GetType()); + return JSONTypeError("decimal string", json_obj.type()); } std::shared_ptr builder() override { return builder_; } @@ -405,20 +488,18 @@ class TimestampConverter final : public ConcreteConverter { builder_ = std::make_shared(type, default_memory_pool()); } - Status AppendValue(const rj::Value& json_obj) override { - if (json_obj.IsNull()) { + Status AppendValue(sj::value& json_obj) override { + if (json_obj.is_null()) { return this->AppendNull(); } int64_t value; - if (json_obj.IsNumber()) { - RETURN_NOT_OK(ConvertNumber(json_obj, *this->type_, &value)); - } else if (json_obj.IsString()) { - std::string_view view(json_obj.GetString(), json_obj.GetStringLength()); + std::string_view view; + if (json_obj.get(view) == simdjson::SUCCESS) { if (!ParseValue(*timestamp_type_, view.data(), view.size(), &value)) { return Status::Invalid("couldn't parse timestamp from ", view); } } else { - return JSONTypeError("timestamp", json_obj.GetType()); + RETURN_NOT_OK(ConvertNumber(json_obj, *this->type_, &value)); } return builder_->Append(value); } @@ -441,21 +522,25 @@ class DayTimeIntervalConverter final builder_ = std::make_shared(default_memory_pool()); } - Status AppendValue(const rj::Value& json_obj) override { - if (json_obj.IsNull()) { + Status AppendValue(sj::value& json_obj) override { + if (json_obj.is_null()) { return this->AppendNull(); } - DayTimeIntervalType::DayMilliseconds value; - if (!json_obj.IsArray()) { - return JSONTypeError("array", json_obj.GetType()); - } - if (json_obj.Size() != 2) { - return Status::Invalid( - "day time interval pair must have exactly two elements, had ", json_obj.Size()); + + sj::array array; + if (json_obj.get(array) != simdjson::SUCCESS) { + return JSONTypeError("array", json_obj.type()); } - RETURN_NOT_OK(ConvertNumber(json_obj[0], *this->type_, &value.days)); - RETURN_NOT_OK( - ConvertNumber(json_obj[1], *this->type_, &value.milliseconds)); + + DayTimeIntervalType::DayMilliseconds value; + RETURN_NOT_OK(ProcessJsonArrayElements( + array, "day-time interval", + [this, &value](sj::value& elem) { + return ConvertNumber(elem, *this->type_, &value.days); + }, + [this, &value](sj::value& elem) { + return ConvertNumber(elem, *this->type_, &value.milliseconds); + })); return builder_->Append(value); } @@ -473,23 +558,28 @@ class MonthDayNanoIntervalConverter final builder_ = std::make_shared(default_memory_pool()); } - Status AppendValue(const rj::Value& json_obj) override { - if (json_obj.IsNull()) { + Status AppendValue(sj::value& json_obj) override { + if (json_obj.is_null()) { return this->AppendNull(); } - MonthDayNanoIntervalType::MonthDayNanos value; - if (!json_obj.IsArray()) { - return JSONTypeError("array", json_obj.GetType()); - } - if (json_obj.Size() != 3) { - return Status::Invalid( - "month_day_nano_interval must have exactly 3 elements, had ", json_obj.Size()); + + sj::array array; + if (json_obj.get(array) != simdjson::SUCCESS) { + return JSONTypeError("array", json_obj.type()); } - RETURN_NOT_OK(ConvertNumber(json_obj[0], *this->type_, &value.months)); - RETURN_NOT_OK(ConvertNumber(json_obj[1], *this->type_, &value.days)); - RETURN_NOT_OK( - ConvertNumber(json_obj[2], *this->type_, &value.nanoseconds)); + MonthDayNanoIntervalType::MonthDayNanos value; + RETURN_NOT_OK(ProcessJsonArrayElements( + array, "month-day-nano interval", + [this, &value](sj::value& elem) { + return ConvertNumber(elem, *this->type_, &value.months); + }, + [this, &value](sj::value& elem) { + return ConvertNumber(elem, *this->type_, &value.days); + }, + [this, &value](sj::value& elem) { + return ConvertNumber(elem, *this->type_, &value.nanoseconds); + })); return builder_->Append(value); } @@ -510,15 +600,15 @@ class StringConverter final Status Init() override { return this->MakeConcreteBuilder(&builder_); } - Status AppendValue(const rj::Value& json_obj) override { - if (json_obj.IsNull()) { + Status AppendValue(sj::value& json_obj) override { + if (json_obj.is_null()) { return this->AppendNull(); } - if (json_obj.IsString()) { - auto view = std::string_view(json_obj.GetString(), json_obj.GetStringLength()); + std::string_view view; + if (json_obj.get(view) == simdjson::SUCCESS) { return builder_->Append(view); } else { - return JSONTypeError("string", json_obj.GetType()); + return JSONTypeError("string", json_obj.type()); } } @@ -541,12 +631,12 @@ class FixedSizeBinaryConverter final Status Init() override { return this->MakeConcreteBuilder(&builder_); } - Status AppendValue(const rj::Value& json_obj) override { - if (json_obj.IsNull()) { + Status AppendValue(sj::value& json_obj) override { + if (json_obj.is_null()) { return this->AppendNull(); } - if (json_obj.IsString()) { - auto view = std::string_view(json_obj.GetString(), json_obj.GetStringLength()); + std::string_view view; + if (json_obj.get(view) == simdjson::SUCCESS) { if (view.length() != static_cast(builder_->byte_width())) { std::stringstream ss; ss << "Invalid string length " << view.length() << " in JSON input for " @@ -555,7 +645,7 @@ class FixedSizeBinaryConverter final } return builder_->Append(view); } else { - return JSONTypeError("string", json_obj.GetType()); + return JSONTypeError("string", json_obj.type()); } } @@ -588,14 +678,21 @@ class VarLengthListLikeConverter final return Status::OK(); } - Status AppendValue(const rj::Value& json_obj) override { - if (json_obj.IsNull()) { + Status AppendValue(sj::value& json_obj) override { + if (json_obj.is_null()) { return this->AppendNull(); } + sj::array array; + if (json_obj.get(array) != simdjson::SUCCESS) { + return JSONTypeError("array", json_obj.type()); + } + size_t num_elements; + if (array.count_elements().get(num_elements) != simdjson::SUCCESS) { + return Status::Invalid("Malformed JSON array for type ", this->type_->ToString()); + } // Extend the child converter with this JSON array - ARROW_ASSIGN_OR_RAISE(auto size, this->SizeOfJSONArray(json_obj)); - RETURN_NOT_OK(builder_->Append(true, size)); - return child_converter_->AppendValues(json_obj); + ARROW_RETURN_NOT_OK(builder_->Append(true, static_cast(num_elements))); + return child_converter_->AppendValues(array).status(); } std::shared_ptr builder() override { return builder_; } @@ -623,29 +720,31 @@ class MapConverter final : public ConcreteConverter { return Status::OK(); } - Status AppendValue(const rj::Value& json_obj) override { - if (json_obj.IsNull()) { + Status AppendValue(sj::value& json_obj) override { + if (json_obj.is_null()) { return this->AppendNull(); } RETURN_NOT_OK(builder_->Append()); - if (!json_obj.IsArray()) { - return JSONTypeError("array", json_obj.GetType()); + sj::array array; + if (json_obj.get(array) != simdjson::SUCCESS) { + return JSONTypeError("array", json_obj.type()); } - auto size = json_obj.Size(); - for (uint32_t i = 0; i < size; ++i) { - const auto& json_pair = json_obj[i]; - if (!json_pair.IsArray()) { - return JSONTypeError("array", json_pair.GetType()); - } - if (json_pair.Size() != 2) { - return Status::Invalid("key item pair must have exactly two elements, had ", - json_pair.Size()); - } - if (json_pair[0].IsNull()) { - return Status::Invalid("null key is invalid"); + + for (auto json_pair : array) { + sj::array json_pair_array; + if (json_pair.get(json_pair_array) != simdjson::SUCCESS) { + return JSONTypeError("array", json_pair.type()); } - RETURN_NOT_OK(key_converter_->AppendValue(json_pair[0])); - RETURN_NOT_OK(item_converter_->AppendValue(json_pair[1])); + + RETURN_NOT_OK(ProcessJsonArrayElements( + json_pair_array, "key-item pair", + [this](sj::value& key) { + if (key.is_null()) { + return Status::Invalid("null key is invalid"); + } + return key_converter_->AppendValue(key); + }, + [this](sj::value& item) { return item_converter_->AppendValue(item); })); } return Status::OK(); } @@ -674,15 +773,19 @@ class FixedSizeListConverter final : public ConcreteConverterAppendNull(); } RETURN_NOT_OK(builder_->Append()); // Extend the child converter with this JSON array - RETURN_NOT_OK(child_converter_->AppendValues(json_obj)); - if (json_obj.GetArray().Size() != static_cast(list_size_)) { - return Status::Invalid("incorrect list size ", json_obj.GetArray().Size()); + sj::array array; + if (json_obj.get(array) != simdjson::SUCCESS) { + return JSONTypeError("array", json_obj.type()); + } + ARROW_ASSIGN_OR_RAISE(int32_t size, child_converter_->AppendValues(array)); + if (size != list_size_) { + return Status::Invalid("incorrect list size ", size); } return Status::OK(); } @@ -710,6 +813,9 @@ class StructConverter final : public ConcreteConverter { child_converters_.push_back(child_converter); child_builders.push_back(child_converter->builder()); } + for (int32_t i = 0; i < type_->num_fields(); ++i) { + field_index_.emplace(type_->field(i)->name(), i); + } builder_ = std::make_shared(type_, default_memory_pool(), std::move(child_builders)); return Status::OK(); @@ -718,45 +824,73 @@ class StructConverter final : public ConcreteConverter { // Append a JSON value that is either an array of N elements in order // or an object mapping struct names to values (omitted struct members // are mapped to null). - Status AppendValue(const rj::Value& json_obj) override { - if (json_obj.IsNull()) { + Status AppendValue(sj::value& json_obj) override { + if (json_obj.is_null()) { return this->AppendNull(); } - if (json_obj.IsArray()) { - auto size = json_obj.Size(); - auto expected_size = static_cast(type_->num_fields()); + sj::array array; + if (json_obj.get(array) == simdjson::SUCCESS) { + auto expected_size = static_cast(type_->num_fields()); + size_t size; + if (array.count_elements().get(size) != simdjson::SUCCESS) { + return Status::Invalid("Malformed JSON array for type ", type_->ToString()); + } if (size != expected_size) { return Status::Invalid("Expected array of size ", expected_size, ", got array of size ", size); } - for (uint32_t i = 0; i < size; ++i) { - RETURN_NOT_OK(child_converters_[i]->AppendValue(json_obj[i])); + size_t i = 0; + for (auto child : array) { + sj::value child_value; + if (child.get(child_value) != simdjson::SUCCESS) { + return Status::Invalid("Could not iterate elements of JSON array"); + } + RETURN_NOT_OK(child_converters_[i]->AppendValue(child_value)); + ++i; } return builder_->Append(); } - if (json_obj.IsObject()) { - auto remaining = json_obj.MemberCount(); - auto num_children = type_->num_fields(); - for (int32_t i = 0; i < num_children; ++i) { - const auto& field = type_->field(i); - auto it = json_obj.FindMember(field->name()); - if (it != json_obj.MemberEnd()) { - --remaining; - RETURN_NOT_OK(child_converters_[i]->AppendValue(it->value)); - } else { - RETURN_NOT_OK(child_converters_[i]->AppendNull()); + sj::object object; + if (json_obj.get(object) == simdjson::SUCCESS) { + // Iterate the object fields in JSON order (the on-demand API is + // forward-only, so per-field lookups would be quadratic and would also + // compare against raw, still-escaped keys). Fields absent from the JSON + // are appended as null afterwards. + auto num_fields = type_->num_fields(); + std::vector field_seen(num_fields, false); + for (auto field_result : object) { + sj::field field; + if (std::move(field_result).get(field) != simdjson::SUCCESS) { + return Status::Invalid("Malformed JSON object for type ", type_->ToString()); } + std::string_view key; + if (field.unescaped_key(/*allow_replacement=*/false).get(key) != + simdjson::SUCCESS) { + return Status::Invalid("Malformed key in JSON object for type ", + type_->ToString()); + } + auto it = field_index_.find(key); + if (it == field_index_.end()) { + return Status::Invalid("Unexpected member \"", key, + "\" in JSON object for type ", type_->ToString()); + } + const int32_t field_num = it->second; + if (field_seen[field_num]) { + return Status::Invalid("Duplicate member \"", key, + "\" in JSON object for type ", type_->ToString()); + } + field_seen[field_num] = true; + sj::value value = field.value(); + RETURN_NOT_OK(child_converters_[field_num]->AppendValue(value)); } - if (remaining > 0) { - rj::StringBuffer sb; - rj::Writer writer(sb); - json_obj.Accept(writer); - return Status::Invalid("Unexpected members in JSON object for type ", - type_->ToString(), " Object: ", sb.GetString()); + for (int32_t i = 0; i < num_fields; ++i) { + if (!field_seen[i]) { + RETURN_NOT_OK(child_converters_[i]->AppendNull()); + } } return builder_->Append(); } - return JSONTypeError("array or object", json_obj.GetType()); + return JSONTypeError("array or object", json_obj.type()); } std::shared_ptr builder() override { return builder_; } @@ -764,6 +898,8 @@ class StructConverter final : public ConcreteConverter { private: std::shared_ptr builder_; std::vector> child_converters_; + // keys are views into the field names owned by type_ + std::unordered_map field_index_; }; // ------------------------------------------------------------------------ @@ -801,40 +937,49 @@ class UnionConverter final : public ConcreteConverter { // Append a JSON value that must be a 2-long array, containing the type_id // and value of the UnionArray's slot. - Status AppendValue(const rj::Value& json_obj) override { - if (json_obj.IsNull()) { + Status AppendValue(sj::value& json_obj) override { + if (json_obj.is_null()) { return this->AppendNull(); } - if (!json_obj.IsArray()) { - return JSONTypeError("array", json_obj.GetType()); - } - if (json_obj.Size() != 2) { - return Status::Invalid("Expected [type_id, value] pair, got array of size ", - json_obj.Size()); - } - const auto& id_obj = json_obj[0]; - if (!id_obj.IsInt()) { - return JSONTypeError("int", id_obj.GetType()); - } - auto id = static_cast(id_obj.GetInt()); - auto child_num = type_id_to_child_num_[id]; - if (child_num == -1) { - return Status::Invalid("type_id ", id, " not found in ", *type_); + sj::array array; + if (json_obj.get(array) != simdjson::SUCCESS) { + return JSONTypeError("array", json_obj.type()); } - auto child_converter = child_converters_[child_num]; - if (mode_ == UnionMode::SPARSE) { - RETURN_NOT_OK(checked_cast(*builder_).Append(id)); - for (auto&& other_converter : child_converters_) { - if (other_converter != child_converter) { - RETURN_NOT_OK(other_converter->AppendNull()); - } - } - } else { - RETURN_NOT_OK(checked_cast(*builder_).Append(id)); - } - return child_converter->AppendValue(json_obj[1]); + int8_t id = 0; + std::shared_ptr child_converter; + + RETURN_NOT_OK(ProcessJsonArrayElements( + array, "[type_id, value] pair", + [this, &id, &child_converter](sj::value& id_elem) { + int64_t id_value; + if (id_elem.get(id_value) != simdjson::SUCCESS) { + return JSONTypeError("int", id_elem.type()); + } + id = static_cast(id_value); + auto child_num = type_id_to_child_num_[id]; + if (child_num == -1) { + return Status::Invalid("type_id ", id, " not found in ", *type_); + } + child_converter = child_converters_[child_num]; + + if (mode_ == UnionMode::SPARSE) { + RETURN_NOT_OK(checked_cast(*builder_).Append(id)); + for (auto&& other_converter : child_converters_) { + if (other_converter != child_converter) { + RETURN_NOT_OK(other_converter->AppendNull()); + } + } + } else { + RETURN_NOT_OK(checked_cast(*builder_).Append(id)); + } + return Status::OK(); + }, + [&child_converter](sj::value& value_elem) { + return child_converter->AppendValue(value_elem); + })); + return Status::OK(); } std::shared_ptr builder() override { return builder_; } @@ -980,15 +1125,28 @@ Result> ArrayFromJSONString(const std::shared_ptr converter; RETURN_NOT_OK(GetConverter(type, &converter)); - rj::Document json_doc; - json_doc.Parse(json_string.data(), json_string.length()); - if (json_doc.HasParseError()) { - return Status::Invalid("JSON parse error at offset ", json_doc.GetErrorOffset(), ": ", - GetParseError_En(json_doc.GetParseError())); + // TODO we should not copy the whole string. Maybe we can move the requirement of + // padding to users of this function + simdjson::padded_string padded_string{json_string}; + + sj::parser parser; + sj::document json_doc; + auto error = parser.iterate(padded_string).get(json_doc); + if (error) { + return Status::Invalid("JSON parse error: ", simdjson::error_message(error)); + } + + sj::array array; + if (json_doc.get(array) != simdjson::SUCCESS) { + return JSONTypeError("array", json_doc.type()); } // The JSON document should be an array, append it - RETURN_NOT_OK(converter->AppendValues(json_doc)); + RETURN_NOT_OK(converter->AppendValues(array)); + // The parser is lazy and does not look past the end of the array by itself + if (!json_doc.at_end()) { + return Status::Invalid("JSON parse error: trailing content after JSON array"); + } std::shared_ptr out; RETURN_NOT_OK(converter->Finish(&out)); return out; @@ -1036,17 +1194,34 @@ Result> ScalarFromJSONString( std::shared_ptr converter; RETURN_NOT_OK(GetConverter(type, &converter)); - rj::Document json_doc; - json_doc.Parse(json_string.data(), json_string.length()); - if (json_doc.HasParseError()) { - return Status::Invalid("JSON parse error at offset ", json_doc.GetErrorOffset(), ": ", - GetParseError_En(json_doc.GetParseError())); + simdjson::padded_string padded_string{ + arrow::internal::JoinToString("[", json_string, "]")}; + + sj::parser parser; + sj::document json_doc; + auto error = parser.iterate(padded_string).get(json_doc); + if (error) { + return Status::Invalid("JSON parse error: ", simdjson::error_message(error)); + } + + sj::array singleton_array; + if (json_doc.get(singleton_array) != simdjson::SUCCESS) { + return JSONTypeError("value", json_doc.type()); + } + + ARROW_ASSIGN_OR_RAISE(int32_t num_elements, converter->AppendValues(singleton_array)); + if (num_elements != 1) { + return Status::Invalid("Expected exactly one JSON value, got ", num_elements, + " values"); + } + // The parser is lazy and does not look past the end of the wrapped value by + // itself, e.g. the input "1] , [2" would otherwise be accepted as 1 + if (!json_doc.at_end()) { + return Status::Invalid("JSON parse error: trailing content after JSON value"); } std::shared_ptr array; - RETURN_NOT_OK(converter->AppendValue(json_doc)); RETURN_NOT_OK(converter->Finish(&array)); - DCHECK_EQ(array->length(), 1); return array->GetScalar(0); } diff --git a/cpp/src/arrow/json/from_string_test.cc b/cpp/src/arrow/json/from_string_test.cc index 654450462e36..817cfe52ee72 100644 --- a/cpp/src/arrow/json/from_string_test.cc +++ b/cpp/src/arrow/json/from_string_test.cc @@ -217,6 +217,9 @@ TYPED_TEST_P(TestIntegersFromString, Errors) { ASSERT_RAISES(Invalid, ArrayFromJSONString(type, "[0.0]")); ASSERT_RAISES(Invalid, ArrayFromJSONString(type, "[\"0\"]")); ASSERT_RAISES(Invalid, ArrayFromJSONString(type, "[[0]]")); + ASSERT_RAISES(Invalid, ArrayFromJSONString(type, "[0]]")); + ASSERT_RAISES(Invalid, ArrayFromJSONString(type, "[0], [1]")); + ASSERT_RAISES(Invalid, ArrayFromJSONString(type, "[0] junk")); } TYPED_TEST_P(TestIntegersFromString, OutOfBounds) { @@ -305,9 +308,9 @@ TYPED_TEST_P(TestStringsFromString, Basics) { AssertJSONArray(type, "[\"\xc3\xa9\"]", {"\xc3\xa9"}); if (!T::is_utf8) { - // Arbitrary binary (non-UTF8) sequence in string + // Arbitrary binary (non-UTF8) sequences cannot be represented s = "\xff\x9f"; - AssertJSONArray(type, "[\"" + s + "\"]", {s}); + ASSERT_RAISES(Invalid, ArrayFromJSONString(type, "[\"" + s + "\"]")); } // Bytes < 0x20 can be represented as JSON unicode escapes @@ -441,7 +444,8 @@ TEST(TestDoubleFromString, Basics) { AssertJSONArray(type, "[1, 2.5, -3e4]", {1.0, 2.5, -3.0e4}); AssertJSONArray(type, "[-0.0, Inf, -Inf, null]", {true, true, true, false}, {-0.0, INFINITY, -INFINITY, 0.0}); - + AssertJSONArray(type, "[Infinity, -Infinity, null]", {true, true, false}, + {INFINITY, -INFINITY, 0.0}); ASSERT_OK_AND_ASSIGN(actual, ArrayFromJSONString(type, "[NaN]")); ASSERT_OK(actual->ValidateFull()); double value = checked_cast(*actual).Value(0); @@ -536,9 +540,10 @@ TEST(TestFixedSizeBinaryFromString, Basics) { {"foo", "bar"}); AssertJSONArray(type, "[null, \"foo\"]", {false, true}, {"", "foo"}); - // Arbitrary binary (non-UTF8) sequence in string + // Arbitrary binary (non-UTF8) sequences cannot be represented: the JSON + // parser requires its input to be valid UTF-8 std::string s = "\xff\x9f\xcc"; - AssertJSONArray(type, "[\"" + s + "\"]", {s}); + ASSERT_RAISES(Invalid, ArrayFromJSONString(type, "[\"" + s + "\"]")); } TEST(TestFixedSizeBinaryFromString, Errors) { @@ -1563,6 +1568,9 @@ TEST(TestScalarFromJSONString, Errors) { ASSERT_RAISES(Invalid, ScalarFromJSONString(binary(), "[]")); ASSERT_RAISES(Invalid, ScalarFromJSONString(boolean(), "0.0")); ASSERT_RAISES(Invalid, ScalarFromJSONString(boolean(), "\"true\"")); + ASSERT_RAISES(Invalid, ScalarFromJSONString(int64(), "1, 2")); + ASSERT_RAISES(Invalid, ScalarFromJSONString(int64(), "1] , [2")); + ASSERT_RAISES(Invalid, ScalarFromJSONString(int64(), "1 junk")); } TEST(TestDictScalarFromJSONString, Basics) { diff --git a/cpp/src/arrow/scalar_test.cc b/cpp/src/arrow/scalar_test.cc index 4096b8cd1788..c9e49ab66abb 100644 --- a/cpp/src/arrow/scalar_test.cc +++ b/cpp/src/arrow/scalar_test.cc @@ -36,6 +36,7 @@ #include "arrow/memory_pool.h" #include "arrow/scalar.h" #include "arrow/status.h" +#include "arrow/testing/builder.h" #include "arrow/testing/extension_type.h" #include "arrow/testing/gtest_util.h" #include "arrow/testing/random.h" @@ -1395,8 +1396,10 @@ class TestListLikeScalar : public ::testing::Test { } { - // Invalid UTF8 in child data - ScalarType scalar(ArrayFromJSON(utf8(), "[null, null, \"\xff\"]")); + std::shared_ptr invalid_utf8; + ArrayFromVector({false, false, true}, {"", "", "\xff"}, + &invalid_utf8); + ScalarType scalar(invalid_utf8); ASSERT_OK(scalar.Validate()); ASSERT_RAISES(Invalid, scalar.ValidateFull()); } diff --git a/cpp/src/arrow/testing/builder.h b/cpp/src/arrow/testing/builder.h index 6beb7760e3bb..2d3cbbe9a458 100644 --- a/cpp/src/arrow/testing/builder.h +++ b/cpp/src/arrow/testing/builder.h @@ -92,6 +92,40 @@ void ArrayFromVector(const std::vector& values, std::shared_ptr* ArrayFromVector(type, values, out); } +// BinaryArrayFromStrings: construct an Array of any binary-like type from +// string values, dispatching on the runtime type id +inline std::shared_ptr BinaryArrayFromStrings( + const std::shared_ptr& type, const std::vector& values) { + std::shared_ptr array; + switch (type->id()) { + case Type::BINARY: + ArrayFromVector(type, values, &array); + break; + case Type::STRING: + ArrayFromVector(type, values, &array); + break; + case Type::LARGE_BINARY: + ArrayFromVector(type, values, &array); + break; + case Type::LARGE_STRING: + ArrayFromVector(type, values, &array); + break; + case Type::BINARY_VIEW: + ArrayFromVector(type, values, &array); + break; + case Type::STRING_VIEW: + ArrayFromVector(type, values, &array); + break; + case Type::FIXED_SIZE_BINARY: + ArrayFromVector(type, values, &array); + break; + default: + ADD_FAILURE() << "unsupported type for binary test data: " << type->ToString(); + break; + } + return array; +} + // ChunkedArrayFromVector: construct a ChunkedArray from vectors of C values template diff --git a/cpp/src/parquet/arrow/arrow_reader_writer_test.cc b/cpp/src/parquet/arrow/arrow_reader_writer_test.cc index 8735aea731ce..78d441b619f6 100644 --- a/cpp/src/parquet/arrow/arrow_reader_writer_test.cc +++ b/cpp/src/parquet/arrow/arrow_reader_writer_test.cc @@ -1495,7 +1495,12 @@ class TestBinaryLikeParquetIO : public ParquetIOTestBase { const std::shared_ptr& fallback_type) { const auto specific_array = ::arrow::ArrayFromJSON(specific_type, json); const auto fallback_array = ::arrow::ArrayFromJSON(fallback_type, json); + CheckRoundTrip(specific_array, fallback_array, binary_type); + } + void CheckRoundTrip(const std::shared_ptr& specific_array, + const std::shared_ptr& fallback_array, + ::arrow::Type::type binary_type) { // When the original Arrow schema isn't stored, the array is decoded as // the fallback type (since there is no specific Parquet logical // type for it). @@ -1521,13 +1526,27 @@ class TestBinaryLikeParquetIO : public ParquetIOTestBase { }; TEST_F(TestBinaryLikeParquetIO, LargeBinary) { - CheckRoundTrip("[\"foo\", \"\", null, \"\xff\"]", ::arrow::Type::LARGE_BINARY, - ::arrow::large_binary(), ::arrow::binary()); + const std::vector is_valid = {true, true, false, true}; + const std::vector values = {"foo", "", "", "\xff"}; + std::shared_ptr specific_array; + ::arrow::ArrayFromVector<::arrow::LargeBinaryType, std::string>(is_valid, values, + &specific_array); + std::shared_ptr fallback_array; + ::arrow::ArrayFromVector<::arrow::BinaryType, std::string>(is_valid, values, + &fallback_array); + CheckRoundTrip(specific_array, fallback_array, ::arrow::Type::LARGE_BINARY); } TEST_F(TestBinaryLikeParquetIO, BinaryView) { - CheckRoundTrip("[\"foo\", \"\", null, \"\xff\"]", ::arrow::Type::BINARY_VIEW, - ::arrow::binary_view(), ::arrow::binary()); + const std::vector is_valid = {true, true, false, true}; + const std::vector values = {"foo", "", "", "\xff"}; + std::shared_ptr specific_array; + ::arrow::ArrayFromVector<::arrow::BinaryViewType, std::string>(is_valid, values, + &specific_array); + std::shared_ptr fallback_array; + ::arrow::ArrayFromVector<::arrow::BinaryType, std::string>(is_valid, values, + &fallback_array); + CheckRoundTrip(specific_array, fallback_array, ::arrow::Type::BINARY_VIEW); } TEST_F(TestBinaryLikeParquetIO, LargeString) { From 2f9559ab3def573adc54910d39e00a43b412f97d Mon Sep 17 00:00:00 2001 From: Alexander Taepper Date: Mon, 27 Jul 2026 13:49:07 +0200 Subject: [PATCH 02/10] fix review comments --- cpp/src/arrow/json/from_string.cc | 52 ++++++++++++++----------------- 1 file changed, 23 insertions(+), 29 deletions(-) diff --git a/cpp/src/arrow/json/from_string.cc b/cpp/src/arrow/json/from_string.cc index 42f8a72ad6e2..6914858a6c00 100644 --- a/cpp/src/arrow/json/from_string.cc +++ b/cpp/src/arrow/json/from_string.cc @@ -139,8 +139,9 @@ class ConcreteConverter : public JSONConverter { int32_t num_elements = 0; for (auto element : json_array) { sj::value value; - if (element.get(value) != simdjson::SUCCESS) { - return Status::Invalid("Could not iterate elements of JSON array"); + if (auto error_code = element.get(value); error_code != simdjson::SUCCESS) { + return Status::Invalid("Could not iterate elements of JSON array: ", + simdjson::error_message(error_code)); } RETURN_NOT_OK(self->AppendValue(value)); num_elements++; @@ -261,49 +262,45 @@ enable_if_unsigned_integer ConvertNumber(sj::value& json_obj, } // Match the std::string_view against NaN, Inf, Infinity with optional leading minus -bool NonFiniteDoubleFromString(std::string_view str, double* out) { - const bool negative = !str.empty() && str.front() == '-'; - if (negative) { - str.remove_prefix(1); - } +std::optional NonFiniteDoubleFromString(std::string_view str) { if (str == "NaN") { - *out = std::numeric_limits::quiet_NaN(); + return std::numeric_limits::quiet_NaN(); + } else if (str == "-NaN") { + return -std::numeric_limits::quiet_NaN(); } else if (str == "Inf" || str == "Infinity") { - *out = std::numeric_limits::infinity(); + return std::numeric_limits::infinity(); + } else if (str == "-Inf" || str == "-Infinity") { + return -std::numeric_limits::infinity(); } else { - return false; - } - if (negative) { - *out = -*out; + return std::nullopt; } - return true; } -bool NonFiniteDoubleFromRawToken(sj::value& json_obj, double* out) { +std::optional NonFiniteDoubleFromRawToken(sj::value& json_obj) { std::string_view token = json_obj.raw_json_token(); // The raw token includes any trailing whitespace up to the next token while (!token.empty() && std::isspace(static_cast(token.back()))) { token.remove_suffix(1); } - return NonFiniteDoubleFromString(token, out); + return NonFiniteDoubleFromString(token); } // Get a double from a JSON value that is either a number, a bare // NaN/Inf/-Inf/Infinity/-Infinity token -bool DoubleFromJsonValue(sj::value& json_obj, double* out) { - if (json_obj.get(*out) == simdjson::SUCCESS) { - return true; +std::optional DoubleFromJsonValue(sj::value& json_obj) { + double value; + if (json_obj.get(value) == simdjson::SUCCESS) { + return value; } - return NonFiniteDoubleFromRawToken(json_obj, out); + return NonFiniteDoubleFromRawToken(json_obj); } // Convert float16/HalfFloatType template enable_if_half_float ConvertNumber(sj::value& json_obj, const DataType& type, uint16_t* out) { - double f64; - if (DoubleFromJsonValue(json_obj, &f64)) { - *out = Float16(f64).bits(); + if (auto f64 = DoubleFromJsonValue(json_obj); f64.has_value()) { + *out = Float16(f64.value()).bits(); return Status::OK(); } *out = static_cast(0); @@ -315,9 +312,8 @@ template enable_if_physical_floating_point ConvertNumber(sj::value& json_obj, const DataType& type, typename T::c_type* out) { - double f64; - if (DoubleFromJsonValue(json_obj, &f64)) { - *out = static_cast(f64); + if (auto f64 = DoubleFromJsonValue(json_obj); f64.has_value()) { + *out = static_cast(f64.value()); return Status::OK(); } *out = static_cast(0); @@ -448,7 +444,7 @@ class DecimalConverter final return this->AppendNull(); } std::string_view string_value; - if (json_obj.get(string_value) == simdjson::SUCCESS) { + if (json_obj.get(string_value) == simdjson::SUCCESS) { int32_t precision, scale; DecimalValue d; RETURN_NOT_OK(DecimalValue::FromString(string_value, &d, &precision, &scale)); @@ -1125,8 +1121,6 @@ Result> ArrayFromJSONString(const std::shared_ptr converter; RETURN_NOT_OK(GetConverter(type, &converter)); - // TODO we should not copy the whole string. Maybe we can move the requirement of - // padding to users of this function simdjson::padded_string padded_string{json_string}; sj::parser parser; From 6b76b3db97c87b0687ef7e20702ebe62937f1026 Mon Sep 17 00:00:00 2001 From: Alexander Taepper Date: Mon, 27 Jul 2026 13:49:57 +0200 Subject: [PATCH 03/10] also add helper method for non-utf8 inputs to python/gdb.cc --- cpp/src/arrow/extension/json_test.cc | 4 +-- python/pyarrow/src/arrow/python/gdb.cc | 46 +++++++++++++++++++++----- 2 files changed, 40 insertions(+), 10 deletions(-) diff --git a/cpp/src/arrow/extension/json_test.cc b/cpp/src/arrow/extension/json_test.cc index 67164def5318..ae0b34db384f 100644 --- a/cpp/src/arrow/extension/json_test.cc +++ b/cpp/src/arrow/extension/json_test.cc @@ -63,10 +63,10 @@ TEST_F(TestJsonExtensionType, JsonRoundtrip) { } TEST_F(TestJsonExtensionType, InvalidUTF8) { - const std::vector invalid_values = {"Ⱥa\xFFⱭ", "Ɽ\xe1\xbdⱤaA"}; for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) { auto json_type = json(storage_type); - auto invalid_input = BinaryArrayFromStrings(storage_type, invalid_values); + auto invalid_input = + BinaryArrayFromStrings(storage_type, {"Ⱥa\xFFⱭ", "Ɽ\xe1\xbdⱤaA"}); auto ext_arr = ExtensionType::WrapArray(json_type, invalid_input); ASSERT_RAISES_WITH_MESSAGE(Invalid, diff --git a/python/pyarrow/src/arrow/python/gdb.cc b/python/pyarrow/src/arrow/python/gdb.cc index 2a7d2eda4bf2..89135239f26b 100644 --- a/python/pyarrow/src/arrow/python/gdb.cc +++ b/python/pyarrow/src/arrow/python/gdb.cc @@ -17,9 +17,13 @@ #include #include +#include +#include #include +#include #include "arrow/array.h" +#include "arrow/array/builder_binary.h" #include "arrow/chunked_array.h" #include "arrow/datum.h" #include "arrow/extension/uuid.h" @@ -29,6 +33,7 @@ #include "arrow/scalar.h" #include "arrow/table.h" #include "arrow/type.h" +#include "arrow/util/checked_cast.h" #include "arrow/util/debug.h" #include "arrow/util/decimal.h" #include "arrow/util/key_value_metadata.h" @@ -43,6 +48,8 @@ using json::ArrayFromJSONString; using json::ChunkedArrayFromJSONString; using json::ScalarFromJSONString; +using ::arrow::internal::checked_cast; + namespace gdb { // Add a nested `arrow` namespace to exercise type lookup from GDB (ARROW-15652) @@ -69,6 +76,25 @@ std::shared_ptr SliceArrayFromJSON(const std::shared_ptr& ty, } } +template +std::shared_ptr BinaryArrayFromStrings( + const std::shared_ptr& type, + const std::vector>& values) { + std::unique_ptr builder; + ARROW_CHECK_OK(MakeBuilder(default_memory_pool(), type, &builder)); + auto& concrete_builder = checked_cast(*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; + ARROW_CHECK_OK(concrete_builder.Finish(&array)); + return array; +} + } // namespace void TestSession() { @@ -448,18 +474,22 @@ void TestSession() { decimal256(50, 6), R"([null, "-123456789012345678901234567890123456789.012345"])"); auto heap_decimal128_array_sliced = heap_decimal128_array->Slice(1, 1); - auto heap_fixed_size_binary_array = - SliceArrayFromJSON(fixed_size_binary(3), "[null, \"abc\", \"\\u0000\\u001f\xff\"]"); + auto heap_fixed_size_binary_array = BinaryArrayFromStrings( + fixed_size_binary(3), {std::nullopt, "abc", std::string("\x00\x1f\xff", 3)}); auto heap_fixed_size_binary_array_zero_width = SliceArrayFromJSON(fixed_size_binary(0), R"([null, ""])"); auto heap_fixed_size_binary_array_sliced = heap_fixed_size_binary_array->Slice(1, 1); - const char* json_binary_array = "[null, \"abcd\", \"\\u0000\\u001f\xff\"]"; - auto heap_binary_array = SliceArrayFromJSON(binary(), json_binary_array); - auto heap_large_binary_array = SliceArrayFromJSON(large_binary(), json_binary_array); - const char* json_string_array = "[null, \"héhé\", \"invalid \xff char\"]"; - auto heap_string_array = SliceArrayFromJSON(utf8(), json_string_array); - auto heap_large_string_array = SliceArrayFromJSON(large_utf8(), json_string_array); + const std::vector> binary_values = { + std::nullopt, "abcd", std::string("\x00\x1f\xff", 3)}; + auto heap_binary_array = BinaryArrayFromStrings(binary(), binary_values); + auto heap_large_binary_array = + BinaryArrayFromStrings(large_binary(), binary_values); + const std::vector> string_values = {std::nullopt, "héhé", + "invalid \xff char"}; + auto heap_string_array = BinaryArrayFromStrings(utf8(), string_values); + auto heap_large_string_array = + BinaryArrayFromStrings(large_utf8(), string_values); auto heap_binary_array_sliced = heap_binary_array->Slice(1, 1); // ChunkedArray From 8627a6fec3f682259724a230bd205f737451c3eb Mon Sep 17 00:00:00 2001 From: Alexander Taepper Date: Tue, 28 Jul 2026 07:41:06 +0200 Subject: [PATCH 04/10] add helpers to fit simdjson into arrow results --- cpp/src/arrow/json/from_string.cc | 331 +++++++++++++++--------------- 1 file changed, 163 insertions(+), 168 deletions(-) diff --git a/cpp/src/arrow/json/from_string.cc b/cpp/src/arrow/json/from_string.cc index 6914858a6c00..75ef4e2196aa 100644 --- a/cpp/src/arrow/json/from_string.cc +++ b/cpp/src/arrow/json/from_string.cc @@ -83,15 +83,65 @@ const char* JsonTypeName(sj::json_type type) { } } -Status JSONTypeError(const char* expected_type, - simdjson::simdjson_result json_type_result) { - sj::json_type json_type; - if (std::move(json_type_result).get(json_type) != simdjson::SUCCESS) { - return Status::Invalid("Expected ", expected_type, - " or null, got malformed JSON value"); +template +inline constexpr bool kAlwaysFalse = false; + +template +const char* JsonTypeName() { + if constexpr (std::is_same_v) { + return "array"; + } else if constexpr (std::is_same_v) { + return "object"; + } else if constexpr (std::is_same_v) { + return "string"; + } else if constexpr (std::is_same_v) { + return "boolean"; + } else if constexpr (std::is_same_v) { + return "null"; + } else if constexpr (std::is_same_v || + std::is_same_v || + std::is_same_v) { + return "number"; + } else { + static_assert(kAlwaysFalse, "unmapped simdjson value type"); + } +} + +template +Result GetAs(sj::value& value) { + SimdjsonValueType typed_value{}; + simdjson::error_code error_code; + if constexpr (std::is_same_v) { + // simdjson has no get<>() for null; probe it explicitly + bool is_null; + error_code = value.is_null().get(is_null); + if (error_code == simdjson::SUCCESS && !is_null) { + error_code = simdjson::INCORRECT_TYPE; + } + } else { + error_code = value.get(typed_value); + } + if (error_code != simdjson::SUCCESS) { + sj::json_type json_type; + if (value.type().get(json_type) != simdjson::SUCCESS) { + return Status::Invalid("Expected ", JsonTypeName(), + " or null, got malformed JSON value"); + } + return Status::Invalid("Expected ", JsonTypeName(), + " or null, got JSON type ", JsonTypeName(json_type)); } - return Status::Invalid("Expected ", expected_type, " or null, got JSON type ", - JsonTypeName(json_type)); + return typed_value; +} + +template +Result Get(simdjson::simdjson_result element, + std::string_view error) { + SimdjsonValueType typed_value; + if (auto error_code = std::move(element).get(typed_value); + error_code != simdjson::SUCCESS) { + return Status::Invalid(error, simdjson::error_message(error_code)); + } + return typed_value; } class JSONConverter { @@ -138,11 +188,9 @@ class ConcreteConverter : public JSONConverter { auto self = static_cast(this); int32_t num_elements = 0; for (auto element : json_array) { - sj::value value; - if (auto error_code = element.get(value); error_code != simdjson::SUCCESS) { - return Status::Invalid("Could not iterate elements of JSON array: ", - simdjson::error_message(error_code)); - } + ARROW_ASSIGN_OR_RAISE( + auto value, + Get(element, "Could not iterate elements of JSON array: ")); RETURN_NOT_OK(self->AppendValue(value)); num_elements++; } @@ -177,10 +225,8 @@ class NullConverter final : public ConcreteConverter { } Status AppendValue(sj::value& json_obj) override { - if (json_obj.is_null()) { - return AppendNull(); - } - return JSONTypeError("null", json_obj.type()); + ARROW_RETURN_NOT_OK(GetAs(json_obj)); + return AppendNull(); } std::shared_ptr builder() override { return builder_; } @@ -203,15 +249,12 @@ class BooleanConverter final : public ConcreteConverter { if (json_obj.is_null()) { return AppendNull(); } - bool bool_value; - if (json_obj.get(bool_value) == simdjson::SUCCESS) { - return builder_->Append(bool_value); - } int64_t int_value; if (json_obj.get(int_value) == simdjson::SUCCESS) { return builder_->Append(int_value != 0); } - return JSONTypeError("boolean", json_obj.type()); + ARROW_ASSIGN_OR_RAISE(bool bool_value, GetAs(json_obj)); + return builder_->Append(bool_value); } std::shared_ptr builder() override { return builder_; } @@ -228,17 +271,13 @@ template enable_if_physical_signed_integer ConvertNumber(sj::value& json_obj, const DataType& type, typename T::c_type* out) { - int64_t v64; - if (json_obj.get(v64) == simdjson::SUCCESS) { - *out = static_cast(v64); - if (*out == v64) { - return Status::OK(); - } else { - return Status::Invalid("Value ", v64, " out of bounds for ", type); - } + *out = static_cast(0); + ARROW_ASSIGN_OR_RAISE(int64_t v64, GetAs(json_obj)); + *out = static_cast(v64); + if (*out == v64) { + return Status::OK(); } else { - *out = static_cast(0); - return JSONTypeError("int", json_obj.type()); + return Status::Invalid("Value ", v64, " out of bounds for ", type); } } @@ -247,17 +286,13 @@ template enable_if_unsigned_integer ConvertNumber(sj::value& json_obj, const DataType& type, typename T::c_type* out) { - uint64_t v64; - if (json_obj.get(v64) == simdjson::SUCCESS) { - *out = static_cast(v64); - if (*out == v64) { - return Status::OK(); - } else { - return Status::Invalid("Value ", v64, " out of bounds for ", type); - } + *out = static_cast(0); + ARROW_ASSIGN_OR_RAISE(uint64_t v64, GetAs(json_obj)); + *out = static_cast(v64); + if (*out == v64) { + return Status::OK(); } else { - *out = static_cast(0); - return JSONTypeError("unsigned int", json_obj.type()); + return Status::Invalid("Value ", v64, " out of bounds for ", type); } } @@ -285,26 +320,18 @@ std::optional NonFiniteDoubleFromRawToken(sj::value& json_obj) { return NonFiniteDoubleFromString(token); } -// Get a double from a JSON value that is either a number, a bare -// NaN/Inf/-Inf/Infinity/-Infinity token -std::optional DoubleFromJsonValue(sj::value& json_obj) { - double value; - if (json_obj.get(value) == simdjson::SUCCESS) { - return value; - } - return NonFiniteDoubleFromRawToken(json_obj); -} - // Convert float16/HalfFloatType template enable_if_half_float ConvertNumber(sj::value& json_obj, const DataType& type, uint16_t* out) { - if (auto f64 = DoubleFromJsonValue(json_obj); f64.has_value()) { + *out = static_cast(0); + if (auto f64 = NonFiniteDoubleFromRawToken(json_obj); f64.has_value()) { *out = Float16(f64.value()).bits(); return Status::OK(); } - *out = static_cast(0); - return JSONTypeError("number", json_obj.type()); + ARROW_ASSIGN_OR_RAISE(auto f64, GetAs(json_obj)); + *out = Float16(f64).bits(); + return arrow::Status::OK(); } // Convert single floating point value @@ -312,12 +339,14 @@ template enable_if_physical_floating_point ConvertNumber(sj::value& json_obj, const DataType& type, typename T::c_type* out) { - if (auto f64 = DoubleFromJsonValue(json_obj); f64.has_value()) { + *out = static_cast(0); + if (auto f64 = NonFiniteDoubleFromRawToken(json_obj); f64.has_value()) { *out = static_cast(f64.value()); return Status::OK(); } - *out = static_cast(0); - return JSONTypeError("number", json_obj.type()); + ARROW_ASSIGN_OR_RAISE(auto f64, GetAs(json_obj)); + *out = static_cast(f64); + return arrow::Status::OK(); } // ------------------------------------------------------------------------ @@ -443,18 +472,15 @@ class DecimalConverter final if (json_obj.is_null()) { return this->AppendNull(); } - std::string_view string_value; - if (json_obj.get(string_value) == simdjson::SUCCESS) { - int32_t precision, scale; - DecimalValue d; - RETURN_NOT_OK(DecimalValue::FromString(string_value, &d, &precision, &scale)); - if (scale != decimal_type_->scale()) { - return Status::Invalid("Invalid scale for decimal: expected ", - decimal_type_->scale(), ", got ", scale); - } - return builder_->Append(d); + ARROW_ASSIGN_OR_RAISE(auto string_value, GetAs(json_obj)); + int32_t precision, scale; + DecimalValue d; + RETURN_NOT_OK(DecimalValue::FromString(string_value, &d, &precision, &scale)); + if (scale != decimal_type_->scale()) { + return Status::Invalid("Invalid scale for decimal: expected ", + decimal_type_->scale(), ", got ", scale); } - return JSONTypeError("decimal string", json_obj.type()); + return builder_->Append(d); } std::shared_ptr builder() override { return builder_; } @@ -523,10 +549,7 @@ class DayTimeIntervalConverter final return this->AppendNull(); } - sj::array array; - if (json_obj.get(array) != simdjson::SUCCESS) { - return JSONTypeError("array", json_obj.type()); - } + ARROW_ASSIGN_OR_RAISE(auto array, GetAs(json_obj)); DayTimeIntervalType::DayMilliseconds value; RETURN_NOT_OK(ProcessJsonArrayElements( @@ -559,10 +582,7 @@ class MonthDayNanoIntervalConverter final return this->AppendNull(); } - sj::array array; - if (json_obj.get(array) != simdjson::SUCCESS) { - return JSONTypeError("array", json_obj.type()); - } + ARROW_ASSIGN_OR_RAISE(auto array, GetAs(json_obj)); MonthDayNanoIntervalType::MonthDayNanos value; RETURN_NOT_OK(ProcessJsonArrayElements( @@ -600,12 +620,9 @@ class StringConverter final if (json_obj.is_null()) { return this->AppendNull(); } - std::string_view view; - if (json_obj.get(view) == simdjson::SUCCESS) { - return builder_->Append(view); - } else { - return JSONTypeError("string", json_obj.type()); - } + + ARROW_ASSIGN_OR_RAISE(auto view, GetAs(json_obj)); + return builder_->Append(view); } std::shared_ptr builder() override { return builder_; } @@ -631,18 +648,14 @@ class FixedSizeBinaryConverter final if (json_obj.is_null()) { return this->AppendNull(); } - std::string_view view; - if (json_obj.get(view) == simdjson::SUCCESS) { - if (view.length() != static_cast(builder_->byte_width())) { - std::stringstream ss; - ss << "Invalid string length " << view.length() << " in JSON input for " - << this->type_->ToString(); - return Status::Invalid(ss.str()); - } - return builder_->Append(view); - } else { - return JSONTypeError("string", json_obj.type()); + ARROW_ASSIGN_OR_RAISE(auto view, GetAs(json_obj)); + if (view.length() != static_cast(builder_->byte_width())) { + std::stringstream ss; + ss << "Invalid string length " << view.length() << " in JSON input for " + << this->type_->ToString(); + return Status::Invalid(ss.str()); } + return builder_->Append(view); } std::shared_ptr builder() override { return builder_; } @@ -678,10 +691,7 @@ class VarLengthListLikeConverter final if (json_obj.is_null()) { return this->AppendNull(); } - sj::array array; - if (json_obj.get(array) != simdjson::SUCCESS) { - return JSONTypeError("array", json_obj.type()); - } + ARROW_ASSIGN_OR_RAISE(auto array, GetAs(json_obj)); size_t num_elements; if (array.count_elements().get(num_elements) != simdjson::SUCCESS) { return Status::Invalid("Malformed JSON array for type ", this->type_->ToString()); @@ -721,16 +731,13 @@ class MapConverter final : public ConcreteConverter { return this->AppendNull(); } RETURN_NOT_OK(builder_->Append()); - sj::array array; - if (json_obj.get(array) != simdjson::SUCCESS) { - return JSONTypeError("array", json_obj.type()); - } + ARROW_ASSIGN_OR_RAISE(auto array, GetAs(json_obj)); - for (auto json_pair : array) { - sj::array json_pair_array; - if (json_pair.get(json_pair_array) != simdjson::SUCCESS) { - return JSONTypeError("array", json_pair.type()); - } + for (auto json_pair_result : array) { + ARROW_ASSIGN_OR_RAISE( + auto json_pair, + Get(json_pair_result, "Could not iterate elements of JSON array: ")); + ARROW_ASSIGN_OR_RAISE(auto json_pair_array, GetAs(json_pair)); RETURN_NOT_OK(ProcessJsonArrayElements( json_pair_array, "key-item pair", @@ -775,10 +782,7 @@ class FixedSizeListConverter final : public ConcreteConverterAppend()); // Extend the child converter with this JSON array - sj::array array; - if (json_obj.get(array) != simdjson::SUCCESS) { - return JSONTypeError("array", json_obj.type()); - } + ARROW_ASSIGN_OR_RAISE(auto array, GetAs(json_obj)); ARROW_ASSIGN_OR_RAISE(int32_t size, child_converter_->AppendValues(array)); if (size != list_size_) { return Status::Invalid("incorrect list size ", size); @@ -837,56 +841,50 @@ class StructConverter final : public ConcreteConverter { } size_t i = 0; for (auto child : array) { - sj::value child_value; - if (child.get(child_value) != simdjson::SUCCESS) { - return Status::Invalid("Could not iterate elements of JSON array"); - } + ARROW_ASSIGN_OR_RAISE( + auto child_value, + Get(child, "Could not iterate elements of JSON array: ")); RETURN_NOT_OK(child_converters_[i]->AppendValue(child_value)); ++i; } return builder_->Append(); } - sj::object object; - if (json_obj.get(object) == simdjson::SUCCESS) { - // Iterate the object fields in JSON order (the on-demand API is - // forward-only, so per-field lookups would be quadratic and would also - // compare against raw, still-escaped keys). Fields absent from the JSON - // are appended as null afterwards. - auto num_fields = type_->num_fields(); - std::vector field_seen(num_fields, false); - for (auto field_result : object) { - sj::field field; - if (std::move(field_result).get(field) != simdjson::SUCCESS) { - return Status::Invalid("Malformed JSON object for type ", type_->ToString()); - } - std::string_view key; - if (field.unescaped_key(/*allow_replacement=*/false).get(key) != - simdjson::SUCCESS) { - return Status::Invalid("Malformed key in JSON object for type ", - type_->ToString()); - } - auto it = field_index_.find(key); - if (it == field_index_.end()) { - return Status::Invalid("Unexpected member \"", key, - "\" in JSON object for type ", type_->ToString()); - } - const int32_t field_num = it->second; - if (field_seen[field_num]) { - return Status::Invalid("Duplicate member \"", key, - "\" in JSON object for type ", type_->ToString()); - } - field_seen[field_num] = true; - sj::value value = field.value(); - RETURN_NOT_OK(child_converters_[field_num]->AppendValue(value)); + ARROW_ASSIGN_OR_RAISE(auto object, GetAs(json_obj)); + // Iterate the object fields in JSON order (the on-demand API is + // forward-only, so per-field lookups would be quadratic and would also + // compare against raw, still-escaped keys). Fields absent from the JSON + // are appended as null afterwards. + auto num_fields = type_->num_fields(); + std::vector field_seen(num_fields, false); + for (auto field_result : object) { + ARROW_ASSIGN_OR_RAISE( + auto field, Get(field_result, "Error getting field of object: ")); + std::string_view key; + if (field.unescaped_key(/*allow_replacement=*/false).get(key) != + simdjson::SUCCESS) { + return Status::Invalid("Malformed key in JSON object for type ", + type_->ToString()); } - for (int32_t i = 0; i < num_fields; ++i) { - if (!field_seen[i]) { - RETURN_NOT_OK(child_converters_[i]->AppendNull()); - } + auto it = field_index_.find(key); + if (it == field_index_.end()) { + return Status::Invalid("Unexpected member \"", key, "\" in JSON object for type ", + type_->ToString()); + } + const int32_t field_num = it->second; + if (field_seen[field_num]) { + return Status::Invalid("Duplicate member \"", key, "\" in JSON object for type ", + type_->ToString()); + } + field_seen[field_num] = true; + sj::value value = field.value(); + RETURN_NOT_OK(child_converters_[field_num]->AppendValue(value)); + } + for (int32_t i = 0; i < num_fields; ++i) { + if (!field_seen[i]) { + RETURN_NOT_OK(child_converters_[i]->AppendNull()); } - return builder_->Append(); } - return JSONTypeError("array or object", json_obj.type()); + return builder_->Append(); } std::shared_ptr builder() override { return builder_; } @@ -938,10 +936,7 @@ class UnionConverter final : public ConcreteConverter { return this->AppendNull(); } - sj::array array; - if (json_obj.get(array) != simdjson::SUCCESS) { - return JSONTypeError("array", json_obj.type()); - } + ARROW_ASSIGN_OR_RAISE(auto array, GetAs(json_obj)); int8_t id = 0; std::shared_ptr child_converter; @@ -949,10 +944,7 @@ class UnionConverter final : public ConcreteConverter { RETURN_NOT_OK(ProcessJsonArrayElements( array, "[type_id, value] pair", [this, &id, &child_converter](sj::value& id_elem) { - int64_t id_value; - if (id_elem.get(id_value) != simdjson::SUCCESS) { - return JSONTypeError("int", id_elem.type()); - } + ARROW_ASSIGN_OR_RAISE(auto id_value, GetAs(id_elem)); id = static_cast(id_value); auto child_num = type_id_to_child_num_[id]; if (child_num == -1) { @@ -1129,11 +1121,12 @@ Result> ArrayFromJSONString(const std::shared_ptr(json_obj)); // The JSON document should be an array, append it RETURN_NOT_OK(converter->AppendValues(array)); @@ -1198,10 +1191,12 @@ Result> ScalarFromJSONString( return Status::Invalid("JSON parse error: ", simdjson::error_message(error)); } - sj::array singleton_array; - if (json_doc.get(singleton_array) != simdjson::SUCCESS) { - return JSONTypeError("value", json_doc.type()); + sj::value json_obj; + if (auto error_code = json_doc.get_value().get(json_obj); + error_code != simdjson::SUCCESS) { + return Status::Invalid("JSON parse error: ", simdjson::error_message(error_code)); } + ARROW_ASSIGN_OR_RAISE(auto singleton_array, GetAs(json_obj)); ARROW_ASSIGN_OR_RAISE(int32_t num_elements, converter->AppendValues(singleton_array)); if (num_elements != 1) { From 16ddada0a5065c8b90b8d34351c9f07d2e1711bd Mon Sep 17 00:00:00 2001 From: Alexander Taepper Date: Tue, 28 Jul 2026 10:20:26 +0200 Subject: [PATCH 05/10] use Status::operator& and linter error --- cpp/src/arrow/json/from_string.cc | 32 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/cpp/src/arrow/json/from_string.cc b/cpp/src/arrow/json/from_string.cc index 75ef4e2196aa..f2d3ea9ccd2a 100644 --- a/cpp/src/arrow/json/from_string.cc +++ b/cpp/src/arrow/json/from_string.cc @@ -88,7 +88,12 @@ inline constexpr bool kAlwaysFalse = false; template const char* JsonTypeName() { - if constexpr (std::is_same_v) { + constexpr bool isNumber = std::is_same_v || + std::is_same_v || + std::is_same_v; + if constexpr (isNumber) { + return "number"; + } else if constexpr (std::is_same_v) { return "array"; } else if constexpr (std::is_same_v) { return "object"; @@ -98,10 +103,6 @@ const char* JsonTypeName() { return "boolean"; } else if constexpr (std::is_same_v) { return "null"; - } else if constexpr (std::is_same_v || - std::is_same_v || - std::is_same_v) { - return "number"; } else { static_assert(kAlwaysFalse, "unmapped simdjson value type"); } @@ -360,38 +361,33 @@ Status ProcessJsonArrayElements(sj::array& json_array, const char* error_context auto end = json_array.end(); size_t index = 0; - Status result = Status::OK(); - - auto process_one = [&](auto&& handler) -> bool { - if (!result.ok()) return false; + auto process_one = [&](auto&& handler) -> arrow::Status { if (it == end) { - result = Status::Invalid(error_context, " must have exactly ", expected_size, - " elements, had ", index); - return false; + return Status::Invalid(error_context, " must have exactly ", expected_size, + " elements, had more"); } sj::value element; auto error = (*it).get(element); if (error) { - result = Status::Invalid("Failed to get element ", index, " from ", error_context); - return false; + return Status::Invalid("Failed to get element ", index, " from ", error_context); } - result = handler(element); + auto result = handler(element); ++it; ++index; - return result.ok(); + return result; }; // Use fold expression to process all handlers in order - (process_one(std::forward(handlers)) && ...); + auto result = (process_one(std::forward(handlers)) & ...); if (!result.ok()) return result; if (it != end) { return Status::Invalid(error_context, " must have exactly ", expected_size, - " elements, had more"); + " elements, had ", index); } return Status::OK(); } From fe4158fc03bafb08aa126eaee5e1e4848ef36e88 Mon Sep 17 00:00:00 2001 From: Alexander Taepper Date: Tue, 28 Jul 2026 10:22:15 +0200 Subject: [PATCH 06/10] improve error context --- cpp/src/arrow/json/from_string.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cpp/src/arrow/json/from_string.cc b/cpp/src/arrow/json/from_string.cc index f2d3ea9ccd2a..cd51fc68a14e 100644 --- a/cpp/src/arrow/json/from_string.cc +++ b/cpp/src/arrow/json/from_string.cc @@ -371,7 +371,8 @@ Status ProcessJsonArrayElements(sj::array& json_array, const char* error_context sj::value element; auto error = (*it).get(element); if (error) { - return Status::Invalid("Failed to get element ", index, " from ", error_context); + return Status::Invalid("Failed to get element ", index, " from ", error_context, + ": ", simdjson::error_message(error)); } auto result = handler(element); From caeb78f1c81519686aa5af0790cda9ea7cc973f0 Mon Sep 17 00:00:00 2001 From: Alexander Taepper Date: Tue, 28 Jul 2026 10:39:29 +0200 Subject: [PATCH 07/10] cleaner function names --- cpp/src/arrow/json/from_string.cc | 74 ++++++++++++++++--------------- 1 file changed, 38 insertions(+), 36 deletions(-) diff --git a/cpp/src/arrow/json/from_string.cc b/cpp/src/arrow/json/from_string.cc index cd51fc68a14e..706695270221 100644 --- a/cpp/src/arrow/json/from_string.cc +++ b/cpp/src/arrow/json/from_string.cc @@ -83,8 +83,8 @@ const char* JsonTypeName(sj::json_type type) { } } -template -inline constexpr bool kAlwaysFalse = false; +// Empty struct to represent the type of a simdjson null value +struct SimdjsonNull {}; template const char* JsonTypeName() { @@ -101,18 +101,18 @@ const char* JsonTypeName() { return "string"; } else if constexpr (std::is_same_v) { return "boolean"; - } else if constexpr (std::is_same_v) { + } else if constexpr (std::is_same_v) { return "null"; } else { - static_assert(kAlwaysFalse, "unmapped simdjson value type"); + static_assert(false, "unmapped simdjson value type"); } } template -Result GetAs(sj::value& value) { +Result GetJsonAs(sj::value& value) { SimdjsonValueType typed_value{}; simdjson::error_code error_code; - if constexpr (std::is_same_v) { + if constexpr (std::is_same_v) { // simdjson has no get<>() for null; probe it explicitly bool is_null; error_code = value.is_null().get(is_null); @@ -135,8 +135,8 @@ Result GetAs(sj::value& value) { } template -Result Get(simdjson::simdjson_result element, - std::string_view error) { +Result GetJsonResult( + simdjson::simdjson_result element, std::string_view error) { SimdjsonValueType typed_value; if (auto error_code = std::move(element).get(typed_value); error_code != simdjson::SUCCESS) { @@ -189,9 +189,9 @@ class ConcreteConverter : public JSONConverter { auto self = static_cast(this); int32_t num_elements = 0; for (auto element : json_array) { - ARROW_ASSIGN_OR_RAISE( - auto value, - Get(element, "Could not iterate elements of JSON array: ")); + ARROW_ASSIGN_OR_RAISE(auto value, + GetJsonResult( + element, "Could not iterate elements of JSON array: ")); RETURN_NOT_OK(self->AppendValue(value)); num_elements++; } @@ -226,7 +226,7 @@ class NullConverter final : public ConcreteConverter { } Status AppendValue(sj::value& json_obj) override { - ARROW_RETURN_NOT_OK(GetAs(json_obj)); + ARROW_RETURN_NOT_OK(GetJsonAs(json_obj)); return AppendNull(); } @@ -254,7 +254,7 @@ class BooleanConverter final : public ConcreteConverter { if (json_obj.get(int_value) == simdjson::SUCCESS) { return builder_->Append(int_value != 0); } - ARROW_ASSIGN_OR_RAISE(bool bool_value, GetAs(json_obj)); + ARROW_ASSIGN_OR_RAISE(bool bool_value, GetJsonAs(json_obj)); return builder_->Append(bool_value); } @@ -273,7 +273,7 @@ enable_if_physical_signed_integer ConvertNumber(sj::value& json_obj, const DataType& type, typename T::c_type* out) { *out = static_cast(0); - ARROW_ASSIGN_OR_RAISE(int64_t v64, GetAs(json_obj)); + ARROW_ASSIGN_OR_RAISE(int64_t v64, GetJsonAs(json_obj)); *out = static_cast(v64); if (*out == v64) { return Status::OK(); @@ -288,7 +288,7 @@ enable_if_unsigned_integer ConvertNumber(sj::value& json_obj, const DataType& type, typename T::c_type* out) { *out = static_cast(0); - ARROW_ASSIGN_OR_RAISE(uint64_t v64, GetAs(json_obj)); + ARROW_ASSIGN_OR_RAISE(uint64_t v64, GetJsonAs(json_obj)); *out = static_cast(v64); if (*out == v64) { return Status::OK(); @@ -330,7 +330,7 @@ enable_if_half_float ConvertNumber(sj::value& json_obj, const DataTyp *out = Float16(f64.value()).bits(); return Status::OK(); } - ARROW_ASSIGN_OR_RAISE(auto f64, GetAs(json_obj)); + ARROW_ASSIGN_OR_RAISE(auto f64, GetJsonAs(json_obj)); *out = Float16(f64).bits(); return arrow::Status::OK(); } @@ -345,7 +345,7 @@ enable_if_physical_floating_point ConvertNumber(sj::value& json_obj, *out = static_cast(f64.value()); return Status::OK(); } - ARROW_ASSIGN_OR_RAISE(auto f64, GetAs(json_obj)); + ARROW_ASSIGN_OR_RAISE(auto f64, GetJsonAs(json_obj)); *out = static_cast(f64); return arrow::Status::OK(); } @@ -469,7 +469,7 @@ class DecimalConverter final if (json_obj.is_null()) { return this->AppendNull(); } - ARROW_ASSIGN_OR_RAISE(auto string_value, GetAs(json_obj)); + ARROW_ASSIGN_OR_RAISE(auto string_value, GetJsonAs(json_obj)); int32_t precision, scale; DecimalValue d; RETURN_NOT_OK(DecimalValue::FromString(string_value, &d, &precision, &scale)); @@ -546,7 +546,7 @@ class DayTimeIntervalConverter final return this->AppendNull(); } - ARROW_ASSIGN_OR_RAISE(auto array, GetAs(json_obj)); + ARROW_ASSIGN_OR_RAISE(auto array, GetJsonAs(json_obj)); DayTimeIntervalType::DayMilliseconds value; RETURN_NOT_OK(ProcessJsonArrayElements( @@ -579,7 +579,7 @@ class MonthDayNanoIntervalConverter final return this->AppendNull(); } - ARROW_ASSIGN_OR_RAISE(auto array, GetAs(json_obj)); + ARROW_ASSIGN_OR_RAISE(auto array, GetJsonAs(json_obj)); MonthDayNanoIntervalType::MonthDayNanos value; RETURN_NOT_OK(ProcessJsonArrayElements( @@ -618,7 +618,7 @@ class StringConverter final return this->AppendNull(); } - ARROW_ASSIGN_OR_RAISE(auto view, GetAs(json_obj)); + ARROW_ASSIGN_OR_RAISE(auto view, GetJsonAs(json_obj)); return builder_->Append(view); } @@ -645,7 +645,7 @@ class FixedSizeBinaryConverter final if (json_obj.is_null()) { return this->AppendNull(); } - ARROW_ASSIGN_OR_RAISE(auto view, GetAs(json_obj)); + ARROW_ASSIGN_OR_RAISE(auto view, GetJsonAs(json_obj)); if (view.length() != static_cast(builder_->byte_width())) { std::stringstream ss; ss << "Invalid string length " << view.length() << " in JSON input for " @@ -688,7 +688,7 @@ class VarLengthListLikeConverter final if (json_obj.is_null()) { return this->AppendNull(); } - ARROW_ASSIGN_OR_RAISE(auto array, GetAs(json_obj)); + ARROW_ASSIGN_OR_RAISE(auto array, GetJsonAs(json_obj)); size_t num_elements; if (array.count_elements().get(num_elements) != simdjson::SUCCESS) { return Status::Invalid("Malformed JSON array for type ", this->type_->ToString()); @@ -728,13 +728,14 @@ class MapConverter final : public ConcreteConverter { return this->AppendNull(); } RETURN_NOT_OK(builder_->Append()); - ARROW_ASSIGN_OR_RAISE(auto array, GetAs(json_obj)); + ARROW_ASSIGN_OR_RAISE(auto array, GetJsonAs(json_obj)); for (auto json_pair_result : array) { ARROW_ASSIGN_OR_RAISE( auto json_pair, - Get(json_pair_result, "Could not iterate elements of JSON array: ")); - ARROW_ASSIGN_OR_RAISE(auto json_pair_array, GetAs(json_pair)); + GetJsonResult(json_pair_result, + "Could not iterate elements of JSON array: ")); + ARROW_ASSIGN_OR_RAISE(auto json_pair_array, GetJsonAs(json_pair)); RETURN_NOT_OK(ProcessJsonArrayElements( json_pair_array, "key-item pair", @@ -779,7 +780,7 @@ class FixedSizeListConverter final : public ConcreteConverterAppend()); // Extend the child converter with this JSON array - ARROW_ASSIGN_OR_RAISE(auto array, GetAs(json_obj)); + ARROW_ASSIGN_OR_RAISE(auto array, GetJsonAs(json_obj)); ARROW_ASSIGN_OR_RAISE(int32_t size, child_converter_->AppendValues(array)); if (size != list_size_) { return Status::Invalid("incorrect list size ", size); @@ -838,15 +839,15 @@ class StructConverter final : public ConcreteConverter { } size_t i = 0; for (auto child : array) { - ARROW_ASSIGN_OR_RAISE( - auto child_value, - Get(child, "Could not iterate elements of JSON array: ")); + ARROW_ASSIGN_OR_RAISE(auto child_value, + GetJsonResult( + child, "Could not iterate elements of JSON array: ")); RETURN_NOT_OK(child_converters_[i]->AppendValue(child_value)); ++i; } return builder_->Append(); } - ARROW_ASSIGN_OR_RAISE(auto object, GetAs(json_obj)); + ARROW_ASSIGN_OR_RAISE(auto object, GetJsonAs(json_obj)); // Iterate the object fields in JSON order (the on-demand API is // forward-only, so per-field lookups would be quadratic and would also // compare against raw, still-escaped keys). Fields absent from the JSON @@ -855,7 +856,8 @@ class StructConverter final : public ConcreteConverter { std::vector field_seen(num_fields, false); for (auto field_result : object) { ARROW_ASSIGN_OR_RAISE( - auto field, Get(field_result, "Error getting field of object: ")); + auto field, + GetJsonResult(field_result, "Error getting field of object: ")); std::string_view key; if (field.unescaped_key(/*allow_replacement=*/false).get(key) != simdjson::SUCCESS) { @@ -933,7 +935,7 @@ class UnionConverter final : public ConcreteConverter { return this->AppendNull(); } - ARROW_ASSIGN_OR_RAISE(auto array, GetAs(json_obj)); + ARROW_ASSIGN_OR_RAISE(auto array, GetJsonAs(json_obj)); int8_t id = 0; std::shared_ptr child_converter; @@ -941,7 +943,7 @@ class UnionConverter final : public ConcreteConverter { RETURN_NOT_OK(ProcessJsonArrayElements( array, "[type_id, value] pair", [this, &id, &child_converter](sj::value& id_elem) { - ARROW_ASSIGN_OR_RAISE(auto id_value, GetAs(id_elem)); + ARROW_ASSIGN_OR_RAISE(auto id_value, GetJsonAs(id_elem)); id = static_cast(id_value); auto child_num = type_id_to_child_num_[id]; if (child_num == -1) { @@ -1123,7 +1125,7 @@ Result> ArrayFromJSONString(const std::shared_ptr(json_obj)); + ARROW_ASSIGN_OR_RAISE(auto array, GetJsonAs(json_obj)); // The JSON document should be an array, append it RETURN_NOT_OK(converter->AppendValues(array)); @@ -1193,7 +1195,7 @@ Result> ScalarFromJSONString( error_code != simdjson::SUCCESS) { return Status::Invalid("JSON parse error: ", simdjson::error_message(error_code)); } - ARROW_ASSIGN_OR_RAISE(auto singleton_array, GetAs(json_obj)); + ARROW_ASSIGN_OR_RAISE(auto singleton_array, GetJsonAs(json_obj)); ARROW_ASSIGN_OR_RAISE(int32_t num_elements, converter->AppendValues(singleton_array)); if (num_elements != 1) { From d31838821a23730186ce36db49ac2d04322c6c9d Mon Sep 17 00:00:00 2001 From: Alexander Taepper Date: Tue, 28 Jul 2026 11:03:18 +0200 Subject: [PATCH 08/10] refactor ProcessJsonArrayElements --- cpp/src/arrow/json/from_string.cc | 147 ++++++++++++++---------------- 1 file changed, 70 insertions(+), 77 deletions(-) diff --git a/cpp/src/arrow/json/from_string.cc b/cpp/src/arrow/json/from_string.cc index 706695270221..f8832718c28e 100644 --- a/cpp/src/arrow/json/from_string.cc +++ b/cpp/src/arrow/json/from_string.cc @@ -15,9 +15,11 @@ // specific language governing permissions and limitations // under the License. +#include #include #include #include +#include #include #include #include @@ -351,44 +353,35 @@ enable_if_physical_floating_point ConvertNumber(sj::value& json_obj, } // ------------------------------------------------------------------------ -// Helper to process a JSON array with exactly N elements, calling a handler for each. -// Each handler is a callable taking sj::value& and returning Status. -template -Status ProcessJsonArrayElements(sj::array& json_array, const char* error_context, - Handlers&&... handlers) { - constexpr size_t expected_size = sizeof...(Handlers); +// Helper to process a JSON array with exactly kExpectedSize elements, calling +// one handler per element, in order. +using JsonElementHandler = std::function; + +template +Status ProcessJsonArrayElements( + sj::array& json_array, const char* error_context, + const std::array& handlers) { auto it = json_array.begin(); auto end = json_array.end(); size_t index = 0; - - auto process_one = [&](auto&& handler) -> arrow::Status { + for (const auto& handler : handlers) { if (it == end) { - return Status::Invalid(error_context, " must have exactly ", expected_size, - " elements, had more"); - } - - sj::value element; - auto error = (*it).get(element); - if (error) { - return Status::Invalid("Failed to get element ", index, " from ", error_context, - ": ", simdjson::error_message(error)); + return Status::Invalid(error_context, " must have exactly ", kExpectedSize, + " elements, had ", index); } - auto result = handler(element); + ARROW_ASSIGN_OR_RAISE( + sj::value element, + GetJsonResult(*it, "Could not iterate elements of JSON array: ")); + RETURN_NOT_OK(handler(element)); ++it; ++index; - return result; - }; - - // Use fold expression to process all handlers in order - auto result = (process_one(std::forward(handlers)) & ...); - - if (!result.ok()) return result; + } if (it != end) { - return Status::Invalid(error_context, " must have exactly ", expected_size, - " elements, had ", index); + return Status::Invalid(error_context, " must have exactly ", kExpectedSize, + " elements, had more"); } return Status::OK(); } @@ -549,14 +542,14 @@ class DayTimeIntervalConverter final ARROW_ASSIGN_OR_RAISE(auto array, GetJsonAs(json_obj)); DayTimeIntervalType::DayMilliseconds value; - RETURN_NOT_OK(ProcessJsonArrayElements( + RETURN_NOT_OK(ProcessJsonArrayElements<2>( array, "day-time interval", - [this, &value](sj::value& elem) { - return ConvertNumber(elem, *this->type_, &value.days); - }, - [this, &value](sj::value& elem) { - return ConvertNumber(elem, *this->type_, &value.milliseconds); - })); + {[this, &value](sj::value& elem) { + return ConvertNumber(elem, *this->type_, &value.days); + }, + [this, &value](sj::value& elem) { + return ConvertNumber(elem, *this->type_, &value.milliseconds); + }})); return builder_->Append(value); } @@ -582,17 +575,17 @@ class MonthDayNanoIntervalConverter final ARROW_ASSIGN_OR_RAISE(auto array, GetJsonAs(json_obj)); MonthDayNanoIntervalType::MonthDayNanos value; - RETURN_NOT_OK(ProcessJsonArrayElements( + RETURN_NOT_OK(ProcessJsonArrayElements<3>( array, "month-day-nano interval", - [this, &value](sj::value& elem) { - return ConvertNumber(elem, *this->type_, &value.months); - }, - [this, &value](sj::value& elem) { - return ConvertNumber(elem, *this->type_, &value.days); - }, - [this, &value](sj::value& elem) { - return ConvertNumber(elem, *this->type_, &value.nanoseconds); - })); + {[this, &value](sj::value& elem) { + return ConvertNumber(elem, *this->type_, &value.months); + }, + [this, &value](sj::value& elem) { + return ConvertNumber(elem, *this->type_, &value.days); + }, + [this, &value](sj::value& elem) { + return ConvertNumber(elem, *this->type_, &value.nanoseconds); + }})); return builder_->Append(value); } @@ -737,15 +730,15 @@ class MapConverter final : public ConcreteConverter { "Could not iterate elements of JSON array: ")); ARROW_ASSIGN_OR_RAISE(auto json_pair_array, GetJsonAs(json_pair)); - RETURN_NOT_OK(ProcessJsonArrayElements( + RETURN_NOT_OK(ProcessJsonArrayElements<2>( json_pair_array, "key-item pair", - [this](sj::value& key) { - if (key.is_null()) { - return Status::Invalid("null key is invalid"); - } - return key_converter_->AppendValue(key); - }, - [this](sj::value& item) { return item_converter_->AppendValue(item); })); + {[this](sj::value& key) { + if (key.is_null()) { + return Status::Invalid("null key is invalid"); + } + return key_converter_->AppendValue(key); + }, + [this](sj::value& item) { return item_converter_->AppendValue(item); }})); } return Status::OK(); } @@ -940,32 +933,32 @@ class UnionConverter final : public ConcreteConverter { int8_t id = 0; std::shared_ptr child_converter; - RETURN_NOT_OK(ProcessJsonArrayElements( + RETURN_NOT_OK(ProcessJsonArrayElements<2>( array, "[type_id, value] pair", - [this, &id, &child_converter](sj::value& id_elem) { - ARROW_ASSIGN_OR_RAISE(auto id_value, GetJsonAs(id_elem)); - id = static_cast(id_value); - auto child_num = type_id_to_child_num_[id]; - if (child_num == -1) { - return Status::Invalid("type_id ", id, " not found in ", *type_); - } - child_converter = child_converters_[child_num]; - - if (mode_ == UnionMode::SPARSE) { - RETURN_NOT_OK(checked_cast(*builder_).Append(id)); - for (auto&& other_converter : child_converters_) { - if (other_converter != child_converter) { - RETURN_NOT_OK(other_converter->AppendNull()); - } - } - } else { - RETURN_NOT_OK(checked_cast(*builder_).Append(id)); - } - return Status::OK(); - }, - [&child_converter](sj::value& value_elem) { - return child_converter->AppendValue(value_elem); - })); + {[this, &id, &child_converter](sj::value& id_elem) { + ARROW_ASSIGN_OR_RAISE(auto id_value, GetJsonAs(id_elem)); + id = static_cast(id_value); + auto child_num = type_id_to_child_num_[id]; + if (child_num == -1) { + return Status::Invalid("type_id ", id, " not found in ", *type_); + } + child_converter = child_converters_[child_num]; + + if (mode_ == UnionMode::SPARSE) { + RETURN_NOT_OK(checked_cast(*builder_).Append(id)); + for (auto&& other_converter : child_converters_) { + if (other_converter != child_converter) { + RETURN_NOT_OK(other_converter->AppendNull()); + } + } + } else { + RETURN_NOT_OK(checked_cast(*builder_).Append(id)); + } + return Status::OK(); + }, + [&child_converter](sj::value& value_elem) { + return child_converter->AppendValue(value_elem); + }})); return Status::OK(); } From cabf13185f69e090e407df218edd815e42a7ac9f Mon Sep 17 00:00:00 2001 From: Alexander Taepper Date: Tue, 28 Jul 2026 16:54:16 +0200 Subject: [PATCH 09/10] do not use static_assert(false,..) which is not portable before C++23 --- cpp/src/arrow/json/from_string.cc | 59 ++++++++++++++++++++----------- 1 file changed, 39 insertions(+), 20 deletions(-) diff --git a/cpp/src/arrow/json/from_string.cc b/cpp/src/arrow/json/from_string.cc index f8832718c28e..f57fad778553 100644 --- a/cpp/src/arrow/json/from_string.cc +++ b/cpp/src/arrow/json/from_string.cc @@ -88,26 +88,45 @@ const char* JsonTypeName(sj::json_type type) { // Empty struct to represent the type of a simdjson null value struct SimdjsonNull {}; -template -const char* JsonTypeName() { - constexpr bool isNumber = std::is_same_v || - std::is_same_v || - std::is_same_v; - if constexpr (isNumber) { - return "number"; - } else if constexpr (std::is_same_v) { - return "array"; - } else if constexpr (std::is_same_v) { - return "object"; - } else if constexpr (std::is_same_v) { - return "string"; - } else if constexpr (std::is_same_v) { - return "boolean"; - } else if constexpr (std::is_same_v) { - return "null"; - } else { - static_assert(false, "unmapped simdjson value type"); - } +template +struct JsonTypeNameOf; + +template <> +struct JsonTypeNameOf { + static constexpr const char* kValue = "array"; +}; +template <> +struct JsonTypeNameOf { + static constexpr const char* kValue = "object"; +}; +template <> +struct JsonTypeNameOf { + static constexpr const char* kValue = "string"; +}; +template <> +struct JsonTypeNameOf { + static constexpr const char* kValue = "boolean"; +}; +template <> +struct JsonTypeNameOf { + static constexpr const char* kValue = "null"; +}; +template <> +struct JsonTypeNameOf { + static constexpr const char* kValue = "number"; +}; +template <> +struct JsonTypeNameOf { + static constexpr const char* kValue = "number"; +}; +template <> +struct JsonTypeNameOf { + static constexpr const char* kValue = "number"; +}; + +template +constexpr const char* JsonTypeName() { + return JsonTypeNameOf::kValue; } template From 2bbad7345d7fc4220b1246702c9f4814b9670284 Mon Sep 17 00:00:00 2001 From: Alexander Taepper Date: Tue, 28 Jul 2026 17:43:22 +0200 Subject: [PATCH 10/10] remove superfluous comment --- cpp/src/arrow/json/from_string.cc | 6 ------ 1 file changed, 6 deletions(-) diff --git a/cpp/src/arrow/json/from_string.cc b/cpp/src/arrow/json/from_string.cc index f57fad778553..9cb14a92a647 100644 --- a/cpp/src/arrow/json/from_string.cc +++ b/cpp/src/arrow/json/from_string.cc @@ -176,12 +176,6 @@ class JSONConverter { Status AppendNull() { return this->builder()->AppendNull(); } - /// Append all elements of `json_array` to this converter's builder and - /// return the number of elements that were appended. The count lets callers - /// with a size expectation (e.g. fixed-size lists or single scalars) - /// validate it without a second pass over the forward-only JSON input. Note - /// that it only counts the elements this call consumed; callers parsing a - /// whole document must check `document::at_end()` to detect trailing input. virtual Result AppendValues(sj::array& json_array) = 0; virtual std::shared_ptr builder() = 0;