-
Notifications
You must be signed in to change notification settings - Fork 221
Added support for JSON ser/de records layout #1275
Added support for JSON ser/de records layout #1275
Conversation
be7e901
to
0625928
Compare
d6d192c
to
33fde65
Compare
Codecov ReportBase: 83.04% // Head: 83.13% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1275 +/- ##
==========================================
+ Coverage 83.04% 83.13% +0.08%
==========================================
Files 363 363
Lines 38442 38884 +442
==========================================
+ Hits 31926 32326 +400
- Misses 6516 6558 +42
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Thoughts on using |
I don't think that's worth another crate pulled in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ready great
My only major comment is to keep things private - i.e. keep all changes needed for the json private to this crate, since they are only used in the context of the crate.
Great PR!!
tests/it/io/json/read.rs
Outdated
}; | ||
|
||
// No idea why assert_eq! doesn't work here, but this does. | ||
assert_eq!(format!("{:?}", expected), format!("{:?}", actual)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect that this is because line 144 uses false
on the nullability of the inner field, but the result is a nullability true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, this was it. Kinda an interesting philosophical question on whether they are actually equal... value-wise, they are identical, even though one could hold nullable values. While I'm not 100% sure I agree with the conclusion, I understand and respect the reasoning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last thought I'll leave here is that the error when they are not equal is pretty confusing:
thread 'io::json::read::read_json_records' panicked at 'assertion failed: `(left == right)`
left: `ListArray[[[1.1, 2, 3], [2, 3], [4, 5, 6]], [[3, 2, 1], [3, 2], [6, 5, 4]]]`,
right: `ListArray[[[1.1, 2, 3], [2, 3], [4, 5, 6]], [[3, 2, 1], [3, 2], [6, 5, 4]]]`', tests/it/io/json/read.rs:114:9
Not sure if the solution is to somehow mark that one is nullable and one is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree - I was also bitten by this many times. The problem challenge is that the ListArray
can become pretty complex if we expose the inner field in the debug :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that makes sense. Especially with deeply nested types.
Co-authored-by: Jorge Leitao <jorgecarleitao@gmail.com>
Co-authored-by: Jorge Leitao <jorgecarleitao@gmail.com>
daec8b7
to
1fb85ec
Compare
Co-authored-by: Jorge Leitao <jorgecarleitao@gmail.com>
ed08dfb
to
06757db
Compare
Thank you so much! Awesome feature and PR! 🙇 |
Prior discussion in #1178
Serialization was easy: individual streaming iterators already produce a record at a time, so we just need to salami slice them to transpose the results.
Deserialization was more complex:
Preallocate
for arrays that have a backing that can be opportunistically resized.MutableListArray
. This is the workhorse that makes later arbitrarily-nested deserialization possible.impl MutableArray
forBox<dyn MutableArray>
. This is mostly trivial delegation, so it may be worth investigating the delegate crate for boilerplate reduction. We need this to support arbitrary recursion at the type level. This allows us to define aMutableListArray<O, Box<dyn MutableArray>>
.It's worth noting that this approach may be useful in general to avoid some of the allocations that the json deserializer currently performs (creating
Vec
of row references).