rok commented on code in PR #51038:
URL: https://github.com/apache/arrow/pull/51038#discussion_r4062275942
##########
cpp/src/arrow/json/parser.cc:
##########
@@ -980,96 +1106,7 @@ class Handler<UnexpectedFieldBehavior::Ignore> : public
HandlerBase {
return DoParse(*this, json);
}
- bool Null() {
- if (Skipping()) {
- return true;
- }
- return HandlerBase::Null();
- }
-
- bool Bool(bool value) {
- if (Skipping()) {
- return true;
- }
- return HandlerBase::Bool(value);
- }
-
- bool RawNumber(const char* data, rj::SizeType size, ...) {
- if (Skipping()) {
- return true;
- }
- return HandlerBase::RawNumber(data, size);
- }
-
- bool String(const char* data, rj::SizeType size, ...) {
- if (Skipping()) {
- return true;
- }
- return HandlerBase::String(data, size);
- }
-
- bool StartObject() {
- ++depth_;
- if (Skipping()) {
- return true;
- }
- return HandlerBase::StartObject();
- }
-
- /// \ingroup rapidjson-handler-interface
- ///
- /// if an unexpected field is encountered, skip until its value has been
consumed
- bool Key(const char* key, rj::SizeType len, ...) {
- MaybeStopSkipping();
- if (Skipping()) {
- return true;
- }
- bool duplicate_keys = false;
- if (ARROW_PREDICT_TRUE(
- SetFieldBuilder(std::string_view(key, len), &duplicate_keys))) {
- return true;
- }
- if (ARROW_PREDICT_FALSE(duplicate_keys)) {
- return false;
- }
- skip_depth_ = depth_;
- return true;
- }
-
- bool EndObject(...) {
- MaybeStopSkipping();
- --depth_;
- if (Skipping()) {
- return true;
- }
- return HandlerBase::EndObject();
- }
-
- bool StartArray() {
- if (Skipping()) {
- return true;
- }
- return HandlerBase::StartArray();
- }
-
- bool EndArray(rj::SizeType size) {
- if (Skipping()) {
- return true;
- }
- return HandlerBase::EndArray(size);
- }
-
- private:
- bool Skipping() { return depth_ >= skip_depth_; }
-
- void MaybeStopSkipping() {
- if (skip_depth_ == depth_) {
- skip_depth_ = std::numeric_limits<int>::max();
- }
- }
-
- int depth_ = 0;
- int skip_depth_ = std::numeric_limits<int>::max();
+ Status HandleUnexpectedField(std::string_view, sj::value) { return
Status::OK(); }
Review Comment:
Am I right to understand `sj.value` is parsed lazily, only if we want to
output it? That would mean we wouldn't catch malformed json in ignored fields,
which was likely the case before. I don't know that we prefer the old behavior,
just noting this has changed. For example, test below would pass before this PR
and pass after.
```diff
diff --git i/cpp/src/arrow/json/parser_test.cc
w/cpp/src/arrow/json/parser_test.cc
index 1b107aa020..c3490f05ff 100644
--- i/cpp/src/arrow/json/parser_test.cc
+++ w/cpp/src/arrow/json/parser_test.cc
@@ -155,6 +155,18 @@ TEST(BlockParserWithSchema, MixedDecimal) {
{R"(["30.04", "1.23"])", R"(["30.001", "1.229"])"});
}
+TEST(BlockParserWithSchema, ValidateIgnoredFields) {
+ auto options = ParseOptions::Defaults();
+ options.explicit_schema = schema({field("known", int64())});
+ options.unexpected_field_behavior = UnexpectedFieldBehavior::Ignore;
+
+ std::shared_ptr<Array> parsed;
+ ASSERT_RAISES(
+ Invalid,
+ ParseFromString(options, R"({"known": 1, "ignored": [1,]})", &parsed)
+ );
+}
```
##########
cpp/src/arrow/json/reader_test.cc:
##########
Review Comment:
What should we do with invalid numeric values (e.g. {"a": 01}) like in the
test below?
```suggestion
TEST(ReaderTest, FailOnMalformedNumbers) {
auto read_options = ReadOptions::Defaults();
auto parse_options = ParseOptions::Defaults();
read_options.use_threads = false;
const std::vector<std::string> malformed = {R"({"a": 01})", R"({"a":
1.})"};
for (const auto& json : malformed) {
auto result = ReadToTable(json, read_options, parse_options);
EXPECT_TRUE(result.status().IsInvalid()) << result.status().ToString();
}
}
```
--
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]
