From 9fc0a86ef7d27c93dd51029a186c61ce68f70384 Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Wed, 11 Nov 2026 17:20:47 +0800 Subject: [PATCH 1/3] GH-55497: [C++] Avoid reusing simdjson values after failed get() --- cpp/src/arrow/json/from_string.cc | 42 +++++++++++++++++-------- cpp/src/arrow/json/from_string_test.cc | 16 ++++++++++ cpp/src/arrow/util/simdjson_internal.cc | 8 ++--- cpp/src/arrow/util/simdjson_internal.h | 37 +++++++--------------- 4 files changed, 60 insertions(+), 43 deletions(-) diff --git a/cpp/src/arrow/json/from_string.cc b/cpp/src/arrow/json/from_string.cc index f573ae710651..d395da619b85 100644 --- a/cpp/src/arrow/json/from_string.cc +++ b/cpp/src/arrow/json/from_string.cc @@ -163,14 +163,19 @@ class BooleanConverter final : public ConcreteConverter { } Status AppendValue(sj::value& json_obj) override { - ARROW_ASSIGN_OR_RAISE(bool is_null, internal::IsJsonNull(json_obj)); - if (is_null) { + ARROW_ASSIGN_OR_RAISE( + auto json_type, internal::ResolveSimdjsonResult(json_obj.type(), + "Failed to determine JSON type")); + if (json_type == sj::json_type::null) { return AppendNull(); } - int64_t int_value; - if (json_obj.get(int_value) == simdjson::SUCCESS) { + if (json_type == sj::json_type::number) { + ARROW_ASSIGN_OR_RAISE(auto int_value, internal::GetJsonAs(json_obj)); return builder_->Append(int_value != 0); } + if (json_type != sj::json_type::boolean) { + return Status::Invalid("Expected JSON boolean for type ", type_->ToString()); + } ARROW_ASSIGN_OR_RAISE(bool bool_value, internal::GetJsonAs(json_obj)); return builder_->Append(bool_value); } @@ -420,18 +425,23 @@ class TimestampConverter final : public ConcreteConverter { } Status AppendValue(sj::value& json_obj) override { - ARROW_ASSIGN_OR_RAISE(bool is_null, internal::IsJsonNull(json_obj)); - if (is_null) { + ARROW_ASSIGN_OR_RAISE( + auto json_type, internal::ResolveSimdjsonResult(json_obj.type(), + "Failed to determine JSON type")); + if (json_type == sj::json_type::null) { return this->AppendNull(); } int64_t value; - std::string_view view; - if (json_obj.get(view) == simdjson::SUCCESS) { + if (json_type == sj::json_type::string) { + ARROW_ASSIGN_OR_RAISE(auto view, internal::GetJsonAs(json_obj)); if (!ParseValue(*timestamp_type_, view.data(), view.size(), &value)) { return Status::Invalid("couldn't parse timestamp from ", view); } - } else { + } else if (json_type == sj::json_type::number) { RETURN_NOT_OK(ConvertNumber(json_obj, *this->type_, &value)); + } else { + return Status::Invalid("Expected JSON string or number for type ", + type_->ToString()); } return builder_->Append(value); } @@ -745,12 +755,14 @@ class StructConverter final : public ConcreteConverter { // or an object mapping struct names to values (omitted struct members // are mapped to null). Status AppendValue(sj::value& json_obj) override { - ARROW_ASSIGN_OR_RAISE(bool is_null, internal::IsJsonNull(json_obj)); - if (is_null) { + ARROW_ASSIGN_OR_RAISE( + auto json_type, internal::ResolveSimdjsonResult(json_obj.type(), + "Failed to determine JSON type")); + if (json_type == sj::json_type::null) { return this->AppendNull(); } - sj::array array; - if (json_obj.get(array) == simdjson::SUCCESS) { + if (json_type == sj::json_type::array) { + ARROW_ASSIGN_OR_RAISE(auto array, internal::GetJsonAs(json_obj)); auto expected_size = static_cast(type_->num_fields()); size_t size; if (array.count_elements().get(size) != simdjson::SUCCESS) { @@ -770,6 +782,10 @@ class StructConverter final : public ConcreteConverter { } return builder_->Append(); } + if (json_type != sj::json_type::object) { + return Status::Invalid("Expected JSON array or object for type ", + type_->ToString()); + } ARROW_ASSIGN_OR_RAISE(auto object, internal::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 diff --git a/cpp/src/arrow/json/from_string_test.cc b/cpp/src/arrow/json/from_string_test.cc index 817cfe52ee72..6398d86e511e 100644 --- a/cpp/src/arrow/json/from_string_test.cc +++ b/cpp/src/arrow/json/from_string_test.cc @@ -1211,6 +1211,22 @@ TEST(TestStructFromString, NestedStruct) { AssertArraysEqual(*expected, *actual); } +TEST(TestStructFromString, NestedStructObjectNotation) { + auto type = + struct_({field("c1", struct_({field("c11", int32()), + field("c12", struct_({field("c121", int32())}))})), + field("c2", int64())}); + constexpr auto json = R"([ + [{"c11": null, "c12": {"c121": -1}}, 1], + [{"c11": 2, "c12": {"c121": null}}, 2] + ])"; + ASSERT_OK_AND_ASSIGN(auto actual, ArrayFromJSONString(type, json)); + ASSERT_OK(actual->ValidateFull()); + ASSERT_OK_AND_ASSIGN( + auto expected, ArrayFromJSONString(type, "[[[null, [-1]], 1], [[2, [null]], 2]]")); + AssertArraysEqual(*expected, *actual); +} + TEST(TestStructFromString, Errors) { auto field_a = field("a", int8()); auto field_b = field("b", boolean()); diff --git a/cpp/src/arrow/util/simdjson_internal.cc b/cpp/src/arrow/util/simdjson_internal.cc index d8cea1a5536c..2e3c5192a40a 100644 --- a/cpp/src/arrow/util/simdjson_internal.cc +++ b/cpp/src/arrow/util/simdjson_internal.cc @@ -515,13 +515,13 @@ const char* JsonTypeName(simdjson::ondemand::json_type type) { } Result IsJsonNull(simdjson::ondemand::value& value) { - bool is_null; - auto error_code = value.is_null().get(is_null); + simdjson::ondemand::json_type json_type; + auto error_code = value.type().get(json_type); if (error_code != simdjson::SUCCESS) { - return Status::Invalid("Error checking for JSON null: ", + return Status::Invalid("Error determining JSON type: ", simdjson::error_message(error_code)); } - return is_null; + return json_type == simdjson::ondemand::json_type::null; } Result MinifyJson(std::string_view json) { diff --git a/cpp/src/arrow/util/simdjson_internal.h b/cpp/src/arrow/util/simdjson_internal.h index fbdf8639dad6..204160789688 100644 --- a/cpp/src/arrow/util/simdjson_internal.h +++ b/cpp/src/arrow/util/simdjson_internal.h @@ -279,40 +279,25 @@ ARROW_EXPORT Result IsJsonNull(simdjson::ondemand::value& value); template Result GetJsonAs(simdjson::ondemand::value& value) { - SimdjsonValueType typed_value{}; - simdjson::error_code error_code; + simdjson::ondemand::json_type json_type; + if (value.type().get(json_type) != simdjson::SUCCESS) { + return Status::Invalid("Expected ", JsonTypeName(), + ", got malformed JSON value"); + } if constexpr (std::same_as) { - // 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; + if (json_type == simdjson::ondemand::json_type::null) { + return SimdjsonNull{}; } + return Status::Invalid("Expected null, got JSON type ", JsonTypeName(json_type)); } else { - error_code = value.get(typed_value); - } - - if (error_code != simdjson::SUCCESS) { - simdjson::ondemand::json_type json_type; - if (value.type().get(json_type) != simdjson::SUCCESS) { - if constexpr (std::same_as) { - return Status::Invalid("Expected null, got malformed JSON value"); - } else { - return Status::Invalid("Expected ", JsonTypeName(), - ", got malformed JSON value"); - } - } - - if constexpr (std::same_as) { - return Status::Invalid("Expected null, got JSON type ", JsonTypeName(json_type)); - } else { + SimdjsonValueType typed_value{}; + if (value.get(typed_value) != simdjson::SUCCESS) { return Status::Invalid("Expected ", JsonTypeName(), ", got JSON type ", JsonTypeName(json_type)); } + return typed_value; } - - return typed_value; } template From 28e322b455e962db4fd52b859ce034d96d60e2ad Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Thu, 12 Nov 2026 16:48:00 +0800 Subject: [PATCH 2/3] address comments --- cpp/src/arrow/json/from_string.cc | 21 ++++++++++++--------- cpp/src/arrow/json/from_string_test.cc | 11 +++++++++++ cpp/src/arrow/util/simdjson_internal.cc | 8 ++++---- cpp/src/arrow/util/simdjson_internal.h | 3 ++- 4 files changed, 29 insertions(+), 14 deletions(-) diff --git a/cpp/src/arrow/json/from_string.cc b/cpp/src/arrow/json/from_string.cc index d395da619b85..a5b77dae48f9 100644 --- a/cpp/src/arrow/json/from_string.cc +++ b/cpp/src/arrow/json/from_string.cc @@ -163,12 +163,13 @@ class BooleanConverter final : public ConcreteConverter { } Status AppendValue(sj::value& json_obj) override { + ARROW_ASSIGN_OR_RAISE(bool is_null, internal::IsJsonNull(json_obj)); + if (is_null) { + return AppendNull(); + } ARROW_ASSIGN_OR_RAISE( auto json_type, internal::ResolveSimdjsonResult(json_obj.type(), "Failed to determine JSON type")); - if (json_type == sj::json_type::null) { - return AppendNull(); - } if (json_type == sj::json_type::number) { ARROW_ASSIGN_OR_RAISE(auto int_value, internal::GetJsonAs(json_obj)); return builder_->Append(int_value != 0); @@ -425,12 +426,13 @@ class TimestampConverter final : public ConcreteConverter { } Status AppendValue(sj::value& json_obj) override { + ARROW_ASSIGN_OR_RAISE(bool is_null, internal::IsJsonNull(json_obj)); + if (is_null) { + return this->AppendNull(); + } ARROW_ASSIGN_OR_RAISE( auto json_type, internal::ResolveSimdjsonResult(json_obj.type(), "Failed to determine JSON type")); - if (json_type == sj::json_type::null) { - return this->AppendNull(); - } int64_t value; if (json_type == sj::json_type::string) { ARROW_ASSIGN_OR_RAISE(auto view, internal::GetJsonAs(json_obj)); @@ -755,12 +757,13 @@ class StructConverter final : public ConcreteConverter { // or an object mapping struct names to values (omitted struct members // are mapped to null). Status AppendValue(sj::value& json_obj) override { + ARROW_ASSIGN_OR_RAISE(bool is_null, internal::IsJsonNull(json_obj)); + if (is_null) { + return this->AppendNull(); + } ARROW_ASSIGN_OR_RAISE( auto json_type, internal::ResolveSimdjsonResult(json_obj.type(), "Failed to determine JSON type")); - if (json_type == sj::json_type::null) { - return this->AppendNull(); - } if (json_type == sj::json_type::array) { ARROW_ASSIGN_OR_RAISE(auto array, internal::GetJsonAs(json_obj)); auto expected_size = static_cast(type_->num_fields()); diff --git a/cpp/src/arrow/json/from_string_test.cc b/cpp/src/arrow/json/from_string_test.cc index 6398d86e511e..73f8d92558d9 100644 --- a/cpp/src/arrow/json/from_string_test.cc +++ b/cpp/src/arrow/json/from_string_test.cc @@ -365,6 +365,17 @@ TEST(TestNullFromString, Errors) { ASSERT_RAISES(Invalid, ArrayFromJSONString(type, "[NaN]")); } +TEST(TestNullFromString, MalformedNull) { + for (const auto& type : {null(), boolean(), int32(), timestamp(TimeUnit::SECOND), + struct_({field("a", int32())})}) { + SCOPED_TRACE(type->ToString()); + for (const auto* json : {"[nul]", "[nullx]"}) { + SCOPED_TRACE(json); + ASSERT_RAISES(Invalid, ArrayFromJSONString(type, json)); + } + } +} + TEST(TestBooleanFromString, Basics) { std::shared_ptr type = boolean(); diff --git a/cpp/src/arrow/util/simdjson_internal.cc b/cpp/src/arrow/util/simdjson_internal.cc index 2e3c5192a40a..d8cea1a5536c 100644 --- a/cpp/src/arrow/util/simdjson_internal.cc +++ b/cpp/src/arrow/util/simdjson_internal.cc @@ -515,13 +515,13 @@ const char* JsonTypeName(simdjson::ondemand::json_type type) { } Result IsJsonNull(simdjson::ondemand::value& value) { - simdjson::ondemand::json_type json_type; - auto error_code = value.type().get(json_type); + bool is_null; + auto error_code = value.is_null().get(is_null); if (error_code != simdjson::SUCCESS) { - return Status::Invalid("Error determining JSON type: ", + return Status::Invalid("Error checking for JSON null: ", simdjson::error_message(error_code)); } - return json_type == simdjson::ondemand::json_type::null; + return is_null; } Result MinifyJson(std::string_view json) { diff --git a/cpp/src/arrow/util/simdjson_internal.h b/cpp/src/arrow/util/simdjson_internal.h index 204160789688..c35c04386d25 100644 --- a/cpp/src/arrow/util/simdjson_internal.h +++ b/cpp/src/arrow/util/simdjson_internal.h @@ -286,7 +286,8 @@ Result GetJsonAs(simdjson::ondemand::value& value) { } if constexpr (std::same_as) { - if (json_type == simdjson::ondemand::json_type::null) { + ARROW_ASSIGN_OR_RAISE(bool is_null, IsJsonNull(value)); + if (is_null) { return SimdjsonNull{}; } return Status::Invalid("Expected null, got JSON type ", JsonTypeName(json_type)); From d4b30c414a64f644ce5842bf52724f1943672676 Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Sun, 15 Nov 2026 23:54:50 +0800 Subject: [PATCH 3/3] minor refactor --- cpp/src/arrow/json/from_string.cc | 12 +++--------- cpp/src/arrow/util/simdjson_internal.h | 12 +++++++----- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/cpp/src/arrow/json/from_string.cc b/cpp/src/arrow/json/from_string.cc index a5b77dae48f9..23f339481981 100644 --- a/cpp/src/arrow/json/from_string.cc +++ b/cpp/src/arrow/json/from_string.cc @@ -167,9 +167,7 @@ class BooleanConverter final : public ConcreteConverter { if (is_null) { return AppendNull(); } - ARROW_ASSIGN_OR_RAISE( - auto json_type, internal::ResolveSimdjsonResult(json_obj.type(), - "Failed to determine JSON type")); + ARROW_ASSIGN_OR_RAISE(auto json_type, internal::GetJsonType(json_obj)); if (json_type == sj::json_type::number) { ARROW_ASSIGN_OR_RAISE(auto int_value, internal::GetJsonAs(json_obj)); return builder_->Append(int_value != 0); @@ -430,9 +428,7 @@ class TimestampConverter final : public ConcreteConverter { if (is_null) { return this->AppendNull(); } - ARROW_ASSIGN_OR_RAISE( - auto json_type, internal::ResolveSimdjsonResult(json_obj.type(), - "Failed to determine JSON type")); + ARROW_ASSIGN_OR_RAISE(auto json_type, internal::GetJsonType(json_obj)); int64_t value; if (json_type == sj::json_type::string) { ARROW_ASSIGN_OR_RAISE(auto view, internal::GetJsonAs(json_obj)); @@ -761,9 +757,7 @@ class StructConverter final : public ConcreteConverter { if (is_null) { return this->AppendNull(); } - ARROW_ASSIGN_OR_RAISE( - auto json_type, internal::ResolveSimdjsonResult(json_obj.type(), - "Failed to determine JSON type")); + ARROW_ASSIGN_OR_RAISE(auto json_type, internal::GetJsonType(json_obj)); if (json_type == sj::json_type::array) { ARROW_ASSIGN_OR_RAISE(auto array, internal::GetJsonAs(json_obj)); auto expected_size = static_cast(type_->num_fields()); diff --git a/cpp/src/arrow/util/simdjson_internal.h b/cpp/src/arrow/util/simdjson_internal.h index c35c04386d25..b582a322a511 100644 --- a/cpp/src/arrow/util/simdjson_internal.h +++ b/cpp/src/arrow/util/simdjson_internal.h @@ -274,16 +274,18 @@ Status VisitJsonValue(simdjson::ondemand::value value, ObjectFn&& object_fn, ARROW_EXPORT const char* JsonTypeName(simdjson::ondemand::json_type type); +// Inspect the type without consuming the value or validating its contents. +inline Result GetJsonType( + simdjson::ondemand::value& value) { + return ResolveSimdjsonResult(value.type(), "Failed to determine JSON type"); +} + // Result because peeking the nonRootScalar can fail (parsed lazily) ARROW_EXPORT Result IsJsonNull(simdjson::ondemand::value& value); template Result GetJsonAs(simdjson::ondemand::value& value) { - simdjson::ondemand::json_type json_type; - if (value.type().get(json_type) != simdjson::SUCCESS) { - return Status::Invalid("Expected ", JsonTypeName(), - ", got malformed JSON value"); - } + ARROW_ASSIGN_OR_RAISE(auto json_type, GetJsonType(value)); if constexpr (std::same_as) { ARROW_ASSIGN_OR_RAISE(bool is_null, IsJsonNull(value));