Rafferty97 commented on PR #9494: URL: https://github.com/apache/arrow-rs/pull/9494#issuecomment-5419833327
Hi @Jefffrey and @alamb, I've now had the time to review each of those test cases individually. Please see my questions and comments below, grouped into a few themes: **Fixes** - `test_read_all` - `test_empty` - `test_ignore_trailing_invalid` - `test_reject_only_null` These tests exercised some edge cases I did miss, so I've fixed those issues and these tests are now green on my branch. Thanks for finding them. **More tolerant parsing** - `test_reject_invalid_json` - `test_line_delimited` My new schema inference implementation uses the `TapeDecoder`, which is much more tolerant when parsing invalid JSON input. For example, it treats commas and new lines as whitespace, as their removal from the grammar doesn't introduce any ambiguity. So, the above test cases are instances where the existing schema inference is stricter than the JSON reader, and my new implementation brings them closer to alignment. With respect to accepting more than just newline-delimited JSON, I think this is a very useful feature of the JSON reader, as it permits parsing regular JSON documents as a single row without having to strip the newlines first. I think, in general, we should prefer not to reject malformed input where a sensible parse does exist and is easy to handle. So, for these two cases, I would suggest either excluding them, or rewriting them to assert a successful schema inference rather than a rejection. What do you think? **Field ordering** - `test_consistent_order` The `test_consistent_order` test case seems like it exposes an inconsistency between `infer_json_schema` and `infer_json_schema_from_iterator`, but I think the reality is more subtle than that. The latter function consumes an iterator of `serde_json::Value`s, which by default store object fields in a `BTreeMap`, which forces them into alphabetical order. However, if the `preserve_order` feature is enabled, they are instead stored in an `IndexMap`, which preserves whatever order the fields were inserted in. So, whether `infer_json_schema` is implemented to preserve the source order of fields, or sorts them alphabetically, it's still going to disagree with `infer_json_schema_from_iterator` in some cases. In light of that, I think preserving the source ordering is the better approach. **Duplicate fields** - `test_duplicate_fields` This is a tricky case to consider because the input in question is malformed and ambiguous, so there's an argument here that the best thing to do is just reject the input. Let me know your thoughts. **Other cases** - `test_complex_null` - I agree that this is a bug in the original code that my PR fixes -- 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]
