diff --git a/cpp/src/arrow/json/from_string.cc b/cpp/src/arrow/json/from_string.cc index f573ae710651..23f339481981 100644 --- a/cpp/src/arrow/json/from_string.cc +++ b/cpp/src/arrow/json/from_string.cc @@ -167,10 +167,14 @@ class BooleanConverter final : public ConcreteConverter { if (is_null) { return AppendNull(); } - int64_t int_value; - if (json_obj.get(int_value) == simdjson::SUCCESS) { + 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); } + 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); } @@ -424,14 +428,18 @@ class TimestampConverter final : public ConcreteConverter { if (is_null) { return this->AppendNull(); } + ARROW_ASSIGN_OR_RAISE(auto json_type, internal::GetJsonType(json_obj)); 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); } @@ -749,8 +757,9 @@ class StructConverter final : public ConcreteConverter { if (is_null) { return this->AppendNull(); } - sj::array array; - if (json_obj.get(array) == simdjson::SUCCESS) { + 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()); size_t size; if (array.count_elements().get(size) != simdjson::SUCCESS) { @@ -770,6 +779,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..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(); @@ -1211,6 +1222,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.h b/cpp/src/arrow/util/simdjson_internal.h index fbdf8639dad6..b582a322a511 100644 --- a/cpp/src/arrow/util/simdjson_internal.h +++ b/cpp/src/arrow/util/simdjson_internal.h @@ -274,45 +274,33 @@ 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) { - SimdjsonValueType typed_value{}; - simdjson::error_code error_code; + ARROW_ASSIGN_OR_RAISE(auto json_type, GetJsonType(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; + 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)); } 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