Add Json `DecoderOptions` and support custom `format_string` for each field #1451

sum12 · 2022-03-16T12:46:10Z

Which issue does this PR close?

add implementation for remaining basic types as needed by the json::decoder

Rationale for this change

This approach is seem cleaner than the on e initially proposed in #1301

What changes are included in this PR?

Non-Formatted parsing for timestamp types is included, but formatted parsing is still missing and can be done as a followup PR.
When serde thinks that the value is a string then these new parsers will kick in and parse the string using provided format or some predefined set of format strings (for timestamp types)
for time32/time64 based types no format is specified

Are there any user-facing changes?

A new format_strings parameter is added to the reader and decoder

alamb · 2022-03-22T22:41:25Z

Will try and check this out tomorrow

alamb

Thanks @sum12 -- sorry for the delayed review

alamb · 2022-03-29T19:18:20Z

arrow/src/json/reader.rs

 //! let file = File::open("test/data/basic.json").unwrap();
 //!
-//! let mut json = json::Reader::new(BufReader::new(file), Arc::new(schema), 1024, None);
+//! let mut json = json::Reader::new(BufReader::new(file), Arc::new(schema), 1024, None, None);


🤔 it is probably time to make some sort of JsonReaderOptions struct so we can add new options without changing the public API

added a DecoderOptions as decoder is like to get those new options in the future than the reader.

arrow/src/json/reader.rs

alamb · 2022-03-29T19:22:27Z

arrow/src/json/reader.rs

        schema: SchemaRef,
        batch_size: usize,
        projection: Option<Vec<String>>,
+        format_strings: Option<HashMap<String, String>>,


I can't help but wonder if we would be better off allowing users to provide their own parsers to support the most general case. For example, what if this was

Suggested change

format_strings: Option<HashMap<String, String>>,

formaters: Option<HashMap<String, Arc<dyn Parser>>,

And then the user could pass whatever custom parser they wanted to the JSON reader?

I often wondered too. Wouldn't it allow the parser to diverge from schema ? We would need a runtime validation of what schema says and formaters actually return.

codecov-commenter · 2022-04-02T12:43:00Z

Codecov Report

Merging #1451 (8427ce6) into master (08f386a) will increase coverage by 0.00%.
The diff coverage is 80.59%.

@@           Coverage Diff           @@
##           master    #1451   +/-   ##
=======================================
  Coverage   82.77%   82.78%           
=======================================
  Files         190      190           
  Lines       54827    54862   +35     
=======================================
+ Hits        45385    45416   +31     
- Misses       9442     9446    +4

Impacted Files	Coverage Δ
arrow/src/util/reader_parser.rs	`67.39% <33.33%> (+2.17%)`	⬆️
arrow/src/json/reader.rs	`83.65% <87.93%> (+0.11%)`	⬆️
arrow/src/datatypes/datatype.rs	`66.40% <0.00%> (-0.40%)`	⬇️
arrow/src/array/transform/mod.rs	`86.35% <0.00%> (-0.12%)`	⬇️
parquet_derive/src/parquet_field.rs	`66.21% <0.00%> (+0.22%)`	⬆️

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 08f386a...8427ce6. Read the comment docs.

the format_string map's key is column name. The value will be used to parse the date64/date32 types from json if the read value is of string type add tests for formatted parser for date{32,64}type for json readers all-parsers start fixup! added format strings (hashmap) to json reader

that way later extensions to the decoder can be added to this struct without breaking API.

sum12 · 2022-04-10T14:19:19Z

@alamb re-review please :-)

alamb

I am sorry for the delay in reviewing @sum12

I think this one is almost ready to go. Thank you for pulling out out the options

I merged to master and pushed a commit to fix the comments

alamb · 2022-04-11T20:55:25Z

arrow/src/json/reader.rs

 pub struct Decoder {
    /// Explicit schema for the JSON file
    schema: SchemaRef,
    /// Optional projection for which columns to load (case-sensitive names)


I think you may have deleted the wrong comment here the /// Batch size .. comment should remain and the /// Optional projection... should be removed

I fixed it in 8427ce6

alamb · 2022-04-11T21:11:51Z

arrow/src/util/reader_parser.rs

    }
 }

+impl Parser for Time64NanosecondType {}


If we do this, doesn't it mean that integers will be parsed as time64s? Is that what you want to allow? If so can you please explain / write a test that covers the behavior (so we don't accidentally break it in the future?)

thanks for pointing that out. Have added tests to parse string from json files to these types. iiuc this latest commit could be also cherry picked to your branch alamb:alamb/batch_size_too ? and this one can be closed.

alamb · 2022-04-12T13:35:29Z

Looks good -- thanks @sum12

implement parser for remaining types used by json decoder

9e363cc

sum12 force-pushed the all-parsers branch from a319e83 to 8ac855c Compare March 16, 2022 12:50

github-actions bot added the arrow Changes to the arrow crate label Mar 16, 2022

alamb mentioned this pull request Mar 29, 2022

Json formatted datetime parsing #1301

Closed

alamb added the api-change Changes to the arrow API label Mar 29, 2022

alamb reviewed Mar 29, 2022

View reviewed changes

sum12 added 2 commits April 2, 2022 14:49

add DecoderOptions struct for holding options for decoder

3202cf3

that way later extensions to the decoder can be added to this struct without breaking API.

sum12 force-pushed the all-parsers branch from fff5ccc to 3202cf3 Compare April 2, 2022 12:52

alamb added 2 commits April 11, 2022 17:01

Merge remote-tracking branch 'apache/master' into all-parsers

573504e

Fixup some comments

8427ce6

alamb approved these changes Apr 11, 2022

View reviewed changes

This was referenced Apr 11, 2022

Consolidate JSON reader options #1538

Closed

Consolidate JSON Reader options and DecoderOptions #1539

Merged

added test for string parsing json reader for time{32,64} types

6986b3e

alamb approved these changes Apr 12, 2022

View reviewed changes

alamb changed the title ~~Allow json reader/decoder to work with format_strings for each field and use Parser trait correspondingly~~ Allow json reader/decoder to work with format_strings for each field Apr 12, 2022

alamb merged commit 68038f5 into apache:master Apr 12, 2022

alamb changed the title ~~Allow json reader/decoder to work with format_strings for each field~~ Add Json DecoderOptions and allow custom format_string for each field Apr 15, 2022

alamb changed the title ~~Add Json DecoderOptions and allow custom format_string for each field~~ Add Json DecoderOptions and support custom format_string for each field Apr 15, 2022

tustvold mentioned this pull request Nov 14, 2022

Parse Time32/Time64 from formatted string #3101

Merged

	format_strings: Option<HashMap<String, String>>,
	formaters: Option<HashMap<String, Arc<dyn Parser>>,

Add Json DecoderOptions and support custom format_string for each field #1451

Add Json DecoderOptions and support custom format_string for each field #1451

Uh oh!

Conversation

sum12 commented Mar 16, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

alamb commented Mar 22, 2022

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Apr 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sum12 commented Apr 10, 2022

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Apr 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add Json `DecoderOptions` and support custom `format_string` for each field #1451

Add Json `DecoderOptions` and support custom `format_string` for each field #1451

codecov-commenter commented Apr 2, 2022 •

edited

Loading