pitrou commented on code in PR #50937:
URL: https://github.com/apache/arrow/pull/50937#discussion_r3841907063
##########
cpp/src/arrow/integration/json_internal.cc:
##########
@@ -1039,24 +1038,28 @@ Result<std::shared_ptr<DataType>> GetTime(const
RjObject& json_type) {
return type;
}
-Result<std::shared_ptr<DataType>> GetDuration(const RjObject& json_type) {
+Result<std::shared_ptr<DataType>> GetDuration(const JsonObject& json_type) {
ARROW_ASSIGN_OR_RAISE(const TimeUnit::type unit,
GetMemberTimeUnit(json_type, "unit"));
return duration(unit);
}
-Result<std::shared_ptr<DataType>> GetTimestamp(const RjObject& json_type) {
+Result<std::shared_ptr<DataType>> GetTimestamp(const JsonObject& json_type) {
ARROW_ASSIGN_OR_RAISE(const TimeUnit::type unit,
GetMemberTimeUnit(json_type, "unit"));
- const auto& it_tz = json_type.FindMember("timezone");
- if (it_tz == json_type.MemberEnd()) {
+ auto timezone_result = json_type["timezone"];
+ if (timezone_result.error() == simdjson::NO_SUCH_FIELD) {
return timestamp(unit);
- } else {
- RETURN_NOT_STRING("timezone", it_tz, json_type);
- return timestamp(unit, it_tz->value.GetString());
}
+
+ ARROW_ASSIGN_OR_RAISE(auto timezone, internal::ResolveSimdjsonResult(
+ timezone_result, "Failed to get
JSON field"));
+ ARROW_ASSIGN_OR_RAISE(
+ auto timezone_string,
+ internal::ResolveSimdjsonResult(timezone.get_string(), "field was not a
string"));
+ return timestamp(unit, std::string(timezone_string));
Review Comment:
The explicit `std::string` cast isn't necessary, is it?
##########
cpp/src/arrow/integration/json_internal.cc:
##########
@@ -1671,49 +1712,46 @@ class ArrayReader {
continue;
}
- DCHECK(val.IsString())
- << "Found non-string JSON value when parsing Decimal128 value";
- DCHECK_GT(val.GetStringLength(), 0)
- << "Empty string found when parsing Decimal128 value";
+ ARROW_ASSIGN_OR_RAISE(auto string,
+ internal::ResolveSimdjsonResult(
+ val.get_string(), "Expected decimal value as
string"));
+
+ DCHECK_GT(string.size(), 0) << "Empty string found when parsing
Decimal128 value";
using Value = typename TypeTraits<T>::ScalarType::ValueType;
- ARROW_ASSIGN_OR_RAISE(Value decimal_val,
Value::FromString(val.GetString()));
+ ARROW_ASSIGN_OR_RAISE(Value decimal_val, Value::FromString(string));
RETURN_NOT_OK(builder.Append(decimal_val));
}
return FinishBuilder(&builder);
}
template <typename T>
- Status GetIntArray(const RjArray& json_array, const int32_t length,
+ Status GetIntArray(const JsonArray& json_array, const int32_t length,
std::shared_ptr<Buffer>* out) {
- if (static_cast<rj::SizeType>(length) != json_array.Size()) {
- return Status::Invalid("Integer array had unexpected length ",
json_array.Size(),
+ if (static_cast<int32_t>(json_array.size()) != length) {
+ return Status::Invalid("Integer array had unexpected length ",
json_array.size(),
" (expected ", length, ")");
}
ARROW_ASSIGN_OR_RAISE(auto buffer, AllocateBuffer(length * sizeof(T),
pool_));
T* values = reinterpret_cast<T*>(buffer->mutable_data());
- for (auto [i, val] : Zip(Enumerate<rj::SizeType>, json_array)) {
+ for (auto [i, val] : Zip(Enumerate<size_t>, json_array)) {
if constexpr (sizeof(T) < sizeof(int64_t)) {
- DCHECK(val.IsInt() || val.IsInt64());
- if (val.IsInt()) {
- values[i] = static_cast<T>(val.GetInt());
- } else {
- values[i] = static_cast<T>(val.GetInt64());
- }
+ ARROW_ASSIGN_OR_RAISE(
+ auto integer,
+ internal::ResolveSimdjsonResult(val.get_int64(), "Expected integer
value"));
+ values[i] = static_cast<T>(integer);
} else {
- // Read 64-bit integers as strings, as JSON numbers cannot represent
- // them exactly.
Review Comment:
Can you keep the comments?
##########
cpp/src/arrow/integration/json_internal.cc:
##########
@@ -880,48 +880,47 @@ Result<TimeUnit::type> GetUnitFromString(const
std::string& unit_str) {
}
template <typename IntType = int>
-Result<IntType> GetMemberInt(const RjObject& obj, const std::string& key) {
- const auto& it = obj.FindMember(key);
- RETURN_NOT_INT(key, it, obj);
- return static_cast<IntType>(it->value.GetInt64());
+Result<IntType> GetMemberInt(const JsonObject& obj, std::string_view key) {
+ ARROW_ASSIGN_OR_RAISE(
+ auto value, internal::ResolveSimdjsonResult(obj[key], "Failed to get
JSON field"));
+ ARROW_ASSIGN_OR_RAISE(auto integer, internal::ResolveSimdjsonResult(
+ value.get_int64(), "field was not an
integer"));
+ return static_cast<IntType>(integer);
}
-Result<bool> GetMemberBool(const RjObject& obj, const std::string& key) {
- const auto& it = obj.FindMember(key);
- RETURN_NOT_BOOL(key, it, obj);
- return it->value.GetBool();
+Result<bool> GetMemberBool(const JsonObject& obj, std::string_view key) {
+ ARROW_ASSIGN_OR_RAISE(
+ auto value, internal::ResolveSimdjsonResult(obj[key], "Failed to get
JSON field"));
+ return internal::ResolveSimdjsonResult(value.get_bool(), "field was not a
boolean");
}
-Result<std::string> GetMemberString(const RjObject& obj, const std::string&
key) {
- const auto& it = obj.FindMember(key);
- RETURN_NOT_STRING(key, it, obj);
- return it->value.GetString();
+Result<std::string> GetMemberString(const JsonObject& obj, std::string_view
key) {
Review Comment:
Perhaps we can return `Result<std::string_view>` here?
##########
cpp/src/arrow/integration/json_internal.cc:
##########
@@ -1407,21 +1442,28 @@ class ArrayReader {
ARROW_ASSIGN_OR_RAISE(const auto json_data_arr, GetDataArray(obj_));
ARROW_ASSIGN_OR_RAISE(const auto json_offsets, GetMemberArray(obj_,
"OFFSET"));
- if (static_cast<int32_t>(json_offsets.Size()) != (length_ + 1)) {
+ if (static_cast<int32_t>(json_offsets.size()) != (length_ + 1)) {
return Status::Invalid(
"JSON OFFSET array size differs from advertised array length + 1");
}
for (auto [i, is_valid, json_val] :
- Zip(Enumerate<rj::SizeType>, is_valid_, json_data_arr)) {
+ Zip(Enumerate<size_t>, is_valid_, json_data_arr)) {
if (!is_valid) {
RETURN_NOT_OK(builder.AppendNull());
continue;
}
ARROW_ASSIGN_OR_RAISE(auto val, GetStringView(json_val));
- int64_t offset_start = ParseOffset(json_offsets[i]);
- int64_t offset_end = ParseOffset(json_offsets[i + 1]);
+ ARROW_ASSIGN_OR_RAISE(auto offset_start_json,
+ internal::ResolveSimdjsonResult(
+ json_offsets.at(i), "Failed to get start
offset"));
+ ARROW_ASSIGN_OR_RAISE(auto offset_end_json,
+ internal::ResolveSimdjsonResult(json_offsets.at(i
+ 1),
+ "Failed to get end
offset"));
Review Comment:
The docs warn that `.at` has linear-time complexity:
https://simdjson.github.io/simdjson/classsimdjson_1_1dom_1_1array.html#a6a6373cc1542a7137b79978680a93d18
We should use an iterator instead, or the [`get_values`
method](https://simdjson.github.io/simdjson/classsimdjson_1_1dom_1_1array.html#a00a9dd263b7628a6965c7e6a25cd96a6)
##########
cpp/src/arrow/integration/json_integration.cc:
##########
@@ -125,44 +127,48 @@ Status IntegrationJsonWriter::WriteRecordBatch(const
RecordBatch& batch) {
class IntegrationJsonReader::Impl {
public:
Impl(MemoryPool* pool, const std::shared_ptr<Buffer>& data)
- : pool_(pool), data_(data), record_batches_(nullptr) {}
+ : pool_(pool), data_(data) {}
Status ParseAndReadSchema() {
- doc_.Parse(reinterpret_cast<const rj::Document::Ch*>(data_->data()),
- static_cast<size_t>(data_->size()));
- if (doc_.HasParseError()) {
- return Status::IOError("JSON parsing failed");
- }
+ ARROW_ASSIGN_OR_RAISE(doc_,
+ internal::ResolveSimdjsonResult(
+ parser_.parse(reinterpret_cast<const
char*>(data_->data()),
+
static_cast<size_t>(data_->size())),
+ "Failed to parse JSON"));
ARROW_ASSIGN_OR_RAISE(schema_, json::ReadSchema(doc_, pool_,
&dictionary_memo_));
- auto it = std::as_const(doc_).FindMember("batches");
- RETURN_NOT_ARRAY("batches", it, doc_);
- record_batches_ = &it->value;
+ ARROW_ASSIGN_OR_RAISE(record_batches_,
+
internal::ResolveSimdjsonResult(doc_["batches"].get_array(),
+ "Failed to get
batches"));
return Status::OK();
}
Result<std::shared_ptr<RecordBatch>> ReadRecordBatch(int i) {
- if (i < 0 || i >= static_cast<int>(record_batches_->GetArray().Size())) {
+ if (i < 0 || i >= static_cast<int>(record_batches_.size())) {
return Status::IndexError("record batch index ", i, " out of bounds");
}
- return json::ReadRecordBatch(record_batches_->GetArray()[i], schema_,
- &dictionary_memo_, pool_);
+
+ ARROW_ASSIGN_OR_RAISE(auto batch,
+
internal::ResolveSimdjsonResult(record_batches_.at(i),
Review Comment:
Same here: the `.at` method has O(i) complexity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
