ARROW-11310: [Rust] implement JSON writer by houqp · Pull Request #9256 · apache/arrow

houqp · 2021-01-19T04:21:38Z

Other than basic type support, this PR also covers the following cases:

github-actions · 2021-01-19T04:22:02Z

https://issues.apache.org/jira/browse/ARROW-11310

codecov-io · 2021-01-19T04:40:04Z

Codecov Report

Merging #9256 (f200959) into master (3e47777) will decrease coverage by 0.01%.
The diff coverage is 84.58%.

@@            Coverage Diff             @@
##           master    #9256      +/-   ##
==========================================
- Coverage   81.89%   81.87%   -0.02%     
==========================================
  Files         215      216       +1     
  Lines       52988    53097     +109     
==========================================
+ Hits        43392    43474      +82     
- Misses       9596     9623      +27

Impacted Files	Coverage Δ
rust/arrow/src/array/array_struct.rs	`88.43% <ø> (ø)`
rust/arrow/src/array/equal/utils.rs	`75.49% <0.00%> (ø)`
rust/arrow/src/bytes.rs	`53.12% <ø> (ø)`
rust/arrow/src/compute/kernels/aggregate.rs	`74.93% <ø> (ø)`
rust/arrow/src/ipc/gen/SparseTensor.rs	`0.00% <0.00%> (ø)`
rust/arrow/src/ipc/gen/Tensor.rs	`0.00% <0.00%> (ø)`
rust/benchmarks/src/bin/nyctaxi.rs	`0.00% <ø> (ø)`
rust/benchmarks/src/bin/tpch.rs	`6.97% <0.00%> (ø)`
rust/datafusion/src/datasource/memory.rs	`79.75% <0.00%> (ø)`
rust/datafusion/src/logical_plan/operators.rs	`75.00% <ø> (ø)`
... and 136 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5665a0c...f200959. Read the comment docs.

jorgecarleitao

Gave this one a quick look, and it looks good so far. I think the nulls are not accounted for.

I left some comments accordingly.

houqp · 2021-01-31T21:48:47Z

 }

-pub fn as_list_array<S: OffsetSizeTrait>(arr: &ArrayRef) -> &GenericListArray<S> {
+pub fn as_generic_list_array<S: OffsetSizeTrait>(arr: &ArrayRef) -> &GenericListArray<S> {


breaking change to make naming more consistent with array type.

houqp · 2021-02-01T07:08:56Z

@jorgecarleitao @nevi-me ready for review.

jorgecarleitao

Great work, @houqp ! This looks super clean and IMO ready to go.

I would add a note to the readme where we discuss json readers. IMO this is a major feature!

jorgecarleitao · 2021-02-01T19:45:45Z

cc @andygrove @alamb @nevi-me @sunchao

nevi-me

Thanks for the implementation @houqp, minor comments

nevi-me · 2021-02-01T21:28:55Z

+                struct_array_to_jsonmap_array(as_struct_array(array), array.len());
+            jsonmaps.into_iter().map(Value::Object).collect()
+        }
+        _ => {


I think there'd be demand for dictionary support, but we can add that separately

Yes I agree -- on both counts :)

yeah, i was planning to add dictionary support, but this patch set grew larger than I expected, so I decided to leave that to a follow up PR for easier review.

alamb

I took a quick skim at this PR and it looks nice to me. Thank you @houqp

…tion

alamb · 2021-02-02T20:50:01Z

I plan to review this PR later today and merge unless I hear otherwise

alamb

Really nice work @houqp

@houqp

…s newline delimited json streams ## Rationale Currently the Arrow json writer makes JSON that looks like this (one record per line): ```json {"foo":1} {"bar":1} ``` Which is not technically valid JSON, which would look something like this: ``` [ {"foo":1}, {"bar":1} ] ``` ## New Features This PR parameterizes the JSON writer so it can write in either format. Note I needed this feature for in IOx, in influxdata/influxdb_iox#870, and I want to propose contributing it back here). ## Other Changes: 1. Added the function `into_inner()` to retrieve the inner writer from the JSON writer, following the model of the Rust standard library (e.g. [BufReader::into_inner](https://doc.rust-lang.org/std/io/struct.BufReader.html#method.into_inner) 2. Per Rust standard pattern, I change the JSON writer so that it doesn't add any Buffering (via `BufReader`) itself, and instead allows the caller the choice of what type of buffering, if any, is needed. 3. Added / cleaned up a bunch of documentation and comments. ## Questions I went with parameterizing the `Writer` output as a trait rather than runtime dispatch, for performance. This shouldn't have backwards compatible issues Given the writer has not yet been released yet (introduced by @houqp #9256) However would people prefer a single `Writer` that took an `Options` struct or something to determine how it wrote out data? Closes #9575 from alamb/alamb/json_arrays Authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>

@houqp

…s newline delimited json streams ## Rationale Currently the Arrow json writer makes JSON that looks like this (one record per line): ```json {"foo":1} {"bar":1} ``` Which is not technically valid JSON, which would look something like this: ``` [ {"foo":1}, {"bar":1} ] ``` ## New Features This PR parameterizes the JSON writer so it can write in either format. Note I needed this feature for in IOx, in influxdata/influxdb_iox#870, and I want to propose contributing it back here). ## Other Changes: 1. Added the function `into_inner()` to retrieve the inner writer from the JSON writer, following the model of the Rust standard library (e.g. [BufReader::into_inner](https://doc.rust-lang.org/std/io/struct.BufReader.html#method.into_inner) 2. Per Rust standard pattern, I change the JSON writer so that it doesn't add any Buffering (via `BufReader`) itself, and instead allows the caller the choice of what type of buffering, if any, is needed. 3. Added / cleaned up a bunch of documentation and comments. ## Questions I went with parameterizing the `Writer` output as a trait rather than runtime dispatch, for performance. This shouldn't have backwards compatible issues Given the writer has not yet been released yet (introduced by @houqp apache/arrow#9256) However would people prefer a single `Writer` that took an `Options` struct or something to determine how it wrote out data? Closes #9575 from alamb/alamb/json_arrays Authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>

github-actions Bot added the Component: Rust label Jan 19, 2021

houqp force-pushed the qp_json_writer branch from 3c9fae3 to 0c68983 Compare January 19, 2021 04:36

houqp marked this pull request as draft January 19, 2021 04:36

jorgecarleitao reviewed Jan 25, 2021

View reviewed changes

Comment thread rust/arrow/src/json/writer.rs Outdated

Comment thread rust/arrow/src/json/writer.rs Outdated

Comment thread rust/arrow/src/json/writer.rs Outdated

Comment thread rust/arrow/src/json/writer.rs Outdated

Comment thread rust/arrow/src/json/writer.rs Outdated

kszucs force-pushed the master branch from d238279 to 437c8c9 Compare January 25, 2021 21:53

houqp added 2 commits January 26, 2021 20:56

ARROW-11310: [Rust] implement JSON writer

fbab6c8

address review feedback

f200959

houqp force-pushed the qp_json_writer branch from 0c68983 to f200959 Compare January 27, 2021 04:56

houqp added 3 commits January 30, 2021 23:20

add list type support

55ddadd

add test for nested struct

255b0ee

add test for struct with list field

3b4f72b

houqp commented Jan 31, 2021

View reviewed changes

houqp added 2 commits January 31, 2021 18:25

suport nested list

e454d2b

support list of struct

6bf7b6e

houqp marked this pull request as ready for review February 1, 2021 05:50

houqp requested a review from jorgecarleitao February 1, 2021 05:51

houqp changed the title ~~WIP: ARROW-11310: [Rust] implement JSON writer~~ ARROW-11310: [Rust] implement JSON writer Feb 1, 2021

fix clippy warnings

db98870

houqp force-pushed the qp_json_writer branch from a6b062f to db98870 Compare February 1, 2021 06:14

jorgecarleitao approved these changes Feb 1, 2021

View reviewed changes

Comment thread rust/arrow/src/array/cast.rs

Comment thread rust/arrow/src/json/writer.rs

nevi-me approved these changes Feb 1, 2021

View reviewed changes

alamb reviewed Feb 1, 2021

View reviewed changes

address feedback, add support for LargeListArray and inline cast func…

1cf556d

…tion

houqp requested a review from alamb February 2, 2021 05:35

alamb approved these changes Feb 2, 2021

View reviewed changes

alamb closed this in 6c880b2 Feb 2, 2021

houqp deleted the qp_json_writer branch February 3, 2021 00:44

alamb mentioned this pull request Feb 25, 2021

ARROW-11773: [Rust] Support writing well formed JSON arrays as well as newline delimited json streams #9575

Closed

asfimport mentioned this pull request Feb 3, 2021

[Rust] Implement arrow JSON writer #27208

Closed

Uh oh!

Conversation

houqp commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jan 19, 2021

Uh oh!

codecov-io commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jorgecarleitao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

houqp Jan 31, 2021

Choose a reason for hiding this comment

Uh oh!

houqp commented Feb 1, 2021

Uh oh!

jorgecarleitao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jorgecarleitao commented Feb 1, 2021

Uh oh!

nevi-me left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nevi-me Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

alamb Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

houqp Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Feb 2, 2021

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

houqp commented Jan 19, 2021 •

edited

Loading

codecov-io commented Jan 19, 2021 •

edited

Loading

houqp Feb 2, 2021 •

edited

Loading