feat(spec): Expose DataFile serde functions#1563
Conversation
CTTY
changed the title
| "referenced_data_file": null, | ||
| "content_offset": null, | ||
| "content_size_in_bytes": null | ||
| }); |
There was a problem hiding this comment.
After some tinkering, I think using serde_json::Value to check the json output gives the most readable test.
Also I found a tricky corner case: DataFile's serializer won't preserve the order of DataFile's array like column_sizes. Right now I just make the array sizes to be one to work around that, maybe there is a better solution?
There was a problem hiding this comment.
Also I found a tricky corner case: DataFile's serializer won't preserve the order of DataFile's array like column_sizes. Right now I just make the array sizes to be one to work around that, maybe there is a better solution?
I guess the randomness comes from HashMap, but since now you use serde_json::Value to do the check, I guess the order doesn't matter?
There was a problem hiding this comment.
Seems it still matters in below outtput. One approach is to change this function to make it determined:
#[cfg(test)]
// sort the keys.
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @CTTY for this PR, generally looks good!
| "referenced_data_file": null, | ||
| "content_offset": null, | ||
| "content_size_in_bytes": null | ||
| }); |
There was a problem hiding this comment.
Also I found a tricky corner case: DataFile's serializer won't preserve the order of DataFile's array like column_sizes. Right now I just make the array sizes to be one to work around that, maybe there is a better solution?
I guess the randomness comes from HashMap, but since now you use serde_json::Value to do the check, I guess the order doesn't matter?
| "referenced_data_file": null, | ||
| "content_offset": null, | ||
| "content_size_in_bytes": null | ||
| }); |
There was a problem hiding this comment.
Seems it still matters in below outtput. One approach is to change this function to make it determined:
#[cfg(test)]
// sort the keys.
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @CTTY for this pr, LGTM!
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @CTTY for this pr, LGTM!
## Which issue does this PR close? - Closes apache#1562 ## What changes are included in this PR? - Added functions to provide serde for DataFile - Have `DataFileSerde::try_from` take in `FormatVersion` directly ## Are these changes tested? Added uts
2 participants
Which issue does this PR close?
What changes are included in this PR?
DataFileSerde::try_fromtake inFormatVersiondirectlyAre these changes tested?
Added uts