Rafferty97 commented on PR #9494:
URL: https://github.com/apache/arrow-rs/pull/9494#issuecomment-5380529752
> i had codex do a comparison with main and found these differences, which i
categorized based on being major/minor differences (these tests assert the
result as of main)
>
> ## major
>
> ```rust
> #[test]
> fn test_read_all() {
> let mut data = "{}\n".repeat(1024);
> data.push_str("{\"late\":true}\n");
> let (schema, _) = infer_json_schema(Cursor::new(data),
None).unwrap();
> assert_eq!(
> schema.field(0),
> &Field::new("late", DataType::Boolean, true)
> );
> }
> ```
>
> - it seems we are only reading the first 1024 records?
>
> ```rust
> #[test]
> fn test_reject_invalid_json() {
> let result = infer_json_schema(Cursor::new(r#"{"a":1 "b":2}"#),
None);
> assert!(result.is_err());
>
> let result = infer_json_schema(Cursor::new(r#"{"a":1e}"#), None);
> assert!(result.is_err());
> }
> ```
>
> - these are some example json which apparently pass schema inference now
>
> ```rust
> #[test]
> fn test_empty() {
> let values = std::iter::empty::<Result<Value, ArrowError>>();
> let schema = infer_json_schema_from_iterator(values).unwrap();
> assert_eq!(schema, Schema::empty());
> }
> ```
>
> - empty input was producing empty schema, but not anymore
>
> ```rust
> #[test]
> fn test_consistent_order() {
> let data = r#"{"z":1,"a":2}"#;
> let (stream_schema, _) = infer_json_schema(Cursor::new(data),
None).unwrap();
> let value = serde_json::from_str::<Value>(data).unwrap();
> let iterator_schema =
infer_json_schema_from_iterator(std::iter::once(Ok(value))).unwrap();
> assert_eq!(stream_schema, iterator_schema);
> }
> ```
>
> - ordering of fields in inferred schema has changed, especially as now the
two public functions aren't consistent with each other
>
> ```rust
> #[test]
> fn test_duplicate_fields() {
> let data = r#"{"a":1,"a":"value"}"#;
> let (schema, _) = infer_json_schema(Cursor::new(data),
None).unwrap();
> assert_eq!(
> schema,
> Schema::new(vec![Field::new("a", DataType::Utf8, true)])
> );
> }
> ```
>
> - duplicate fields are being inferred as two different fields; previous
behaviour was to use latter value, though maybe that is also confusing
>
> ## minor
>
> `max_read_records` seems to be applied after reading, so it technically
can overshoot how many records were read (even if theyre not used in
inference). this could be surprising for consumers using `infer_json_schema()`
as it may now move the cursor more than before. another consequence of this is
invalid trailing data isnt ignored, for example:
>
> ```rust
> #[test]
> fn test_ignore_trailing_invalid() {
> let data = b"{\"a\":1}\nthis is not JSON\n";
> let (schema, record_count) = infer_json_schema(Cursor::new(data),
Some(1)).unwrap();
> assert_eq!(record_count, 1);
> assert_eq!(schema.field(0), &Field::new("a", DataType::Int64,
true));
> }
> ```
>
> - we expect to read only one record, but since we read beyond that it
errors
>
> ```rust
> #[test]
> fn test_line_delimited() {
> let result = infer_json_schema(Cursor::new(r#"{"a":1} {"b":2}"#),
None);
> assert!(result.is_err());
> }
> ```
>
> - this ties into previous points, but it seems we now allow parsing json
records that arent newline delimited; it seems we didnt exactly document that
we only expect newline delimited json however 🤔
>
> ```rust
> #[test]
> fn test_complex_null() {
> let result =
infer_json_schema(Cursor::new(r#"{"a":[null,{"b":1}]}"#), None);
> assert!(result.is_err());
> }
> ```
>
> - nulls in a complex object are now allowed - this looks to be a bug in
our current code actually
>
> ```rust
> #[test]
> fn test_reject_only_null() {
> let result = infer_json_schema(Cursor::new("null\n"), None);
> assert!(result.is_err());
> }
> ```
>
> - only `null` input seems to be accepted instead of rejected
>
> i havent taken too close a look at the code in this PR, but it does seem
quite a bit of behaviour is changing (even if the benchmark numbers are pretty
good 🚀). it is to be said though that our existing test suite is quite lacking
to have not asserted these cases
Thanks for taking the time to review the PR. I'm a little surprised to see
this many changes in behaviour, but when I get that chance I'll review them and
either fix or justify these cases.
--
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]
