-
Notifications
You must be signed in to change notification settings - Fork 850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate old JSON reader (#3610) #3718
Conversation
d219f13
to
bbf0d5e
Compare
@@ -1121,7 +1121,7 @@ mod tests { | |||
// Note: we are using the JSON Arrow reader for brevity | |||
let json_content = r#" | |||
{"stocks":{"long": "$AAA", "short": "$BBB"}} | |||
{"stocks":{"long": null, "long": "$CCC", "short": null}} | |||
{"stocks":{"long": "$CCC", "short": null}} |
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 wasn't actually valid JSON, it contained a duplicate key - the RawReader handles this differently (preserving the duplicate) than the old JSON reader which preserves only the last instance
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 sounds like behavior change. Do we have document it at RawReader
?
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 prior behaviour wasn't documented on Decoder
, so I think this is fine
/// [`RawDecoder`]: crate::raw::RawDecoder | ||
/// [#3610]: https://github.com/apache/arrow-rs/issues/3610 | ||
#[derive(Debug)] | ||
#[deprecated(note = "Use RawDecoder instead")] |
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.
Still need the comment? Actually it reads unclear and seems strange to put issue link there.
/// [`RawDecoder`]: crate::raw::RawDecoder | |
/// [#3610]: https://github.com/apache/arrow-rs/issues/3610 | |
#[derive(Debug)] | |
#[deprecated(note = "Use RawDecoder instead")] | |
#[derive(Debug)] | |
#[deprecated(note = "Use RawDecoder instead")] |
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.
/// [`RawReader`]: crate::raw::RawReader | ||
/// [#3610]: https://github.com/apache/arrow-rs/issues/3610 |
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.
ditto.
/// [`RawReader`]: crate::raw::RawReader | |
/// [#3610]: https://github.com/apache/arrow-rs/issues/3610 |
Benchmark runs are scheduled for baseline = 61ea9f2 and contender = 6bf0aab. 6bf0aab is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Part of #3610
Rationale for this change
Deprecate the old reader to encourage people to switch to the new reader
What changes are included in this PR?
Are there any user-facing changes?