Support String Coercion in Raw JSON Reader #3736

rguerreiromsft · 2023-02-20T06:09:41Z

Which issue does this PR close?

None. This is a feature request.

Rationale for this change

infer_json_schema coerces primitive values into string, and it couldn't work when reading the same json with the given schema.

        let schema = Schema::new(vec![
            Field::new("c1", DataType::Utf8, true),
            Field::new("c2", DataType::Utf8, true),
        ]);

        let inferred_schema = infer_json_schema_from_iterator(
            vec![
                Ok(serde_json::json!({"c1": 1, "c2": 1})),
                Ok(serde_json::json!({"c1": "a", "c2": 0})),
                Ok(serde_json::json!({"c1": true, "c2": "ok"})),
            ]
            .into_iter(),
        )
        .unwrap();

        assert_eq!(inferred_schema, schema);

What changes are included in this PR?

Add flag to make RawReader behave the same as infer_json_schema.

Are there any user-facing changes?

Yes, the user can opt-in a coerce_primitive flag.

rguerreiromsft · 2023-02-20T06:29:23Z

I'm not really happy with the Loose configuration I implemented because the string conversion to bool doesn't make much sense. And bool doesn't follow javascript either, where it considers an empty list to be false.

Maybe a best implementation for this is to make bools and numbers as they used to be, and consider the loose behaviour to just parse everything to string, because in that case we don't lose any information.

The good thing about this implementation is that if the end-user wants to get their hands dirty, they can implement their custom converter and get it tuned to their needs.

tustvold

As written I don't think TapeConverter can be implemented outside the crate as Tape is not public. I happen to think this is a good thing as I would rather what are effectively implementation details not be exposed outside of the crate.

I wonder if we could rework the generics slightly so that TapeConverter is a type-erased implementation detail of the various ArrayDecoder, instead of plumbing it the whole way through?

I do also wonder if we really need generics for this, or if a simple strict flag is all that is needed....

tustvold · 2023-02-20T11:50:10Z

arrow-json/src/raw/tape.rs

@@ -87,14 +92,15 @@ impl Display for TapeElement {
 ///
 /// [simdjson]: https://github.com/simdjson/simdjson/blob/master/doc/tape.md
 #[derive(Debug)]
-pub struct Tape<'a> {
+pub struct Tape<'a, C: TapeConverter> {


I think it would make for a cleaner interface if the converter were instead given the tape instead of Tape being parameterised on TapeConverter. This would allow ArrayDecoder and by extension RawDecoder / RawReader to not need a generic

tustvold · 2023-02-20T11:54:56Z

arrow-json/src/raw/converter.rs

+        TapeElement::True => Ok("true".into()),
+        TapeElement::False => Ok("false".into()),
+        TapeElement::Number(idx) => Ok(tape.get_string(idx).into()),
+        TapeElement::String(idx) => Ok(format!("\"{}\"", tape.get_string(idx)).into()),


I'm not sure this will correctly escape newlines or quotes?

Good catch. Do you think using serde_json is a good approach to solve this?

To be completely honest I struggle to understand the use-case, the performance will be bad and it seems like a rather strange API... This sort of schema coercion I'm not sure really belongs in the reader

The use case is that we don't always control the json format we're reading from, but we can control the Schema.

And it's the behaviour given by infer_json_schema:

let schema = Schema::new(vec![ Field::new("c1", DataType::Utf8, true), Field::new("c2", DataType::Utf8, true), ]); let inferred_schema = infer_json_schema_from_iterator( vec![ Ok(serde_json::json!({"c1": 1, "c2": 1})), Ok(serde_json::json!({"c1": "a", "c2": 0})), Ok(serde_json::json!({"c1": true, "c2": "ok"})), ] .into_iter(), ) .unwrap(); assert_eq!(inferred_schema, schema);

And it's the behaviour given by infer_json_schema:

infer_json_schema coerces primitives, I'm not aware of it coercing objects or lists to strings? If it is, that is not intentional

The use case is that we don't always control the json format we're reading from, but we can control the Schema.

Does each file have a consistent schema though, i.e. does a given column have a mix of objects and primitives in the same file? The assumption of arrow typically is that schema evolution occurs at the file granularity, and can therefore be handled at a higher level...

Oh, no, it's just for primitives, and only to strings, if I'm not mistaken. Let me see what it coerces between number and bool.

I probably just got creative there and asked why not? But thinking about it, what I really need is to be able to read json with the schema that was inferred before, and use it to save to Parquet.

Maybe we don't even need a flag for it, shouldn't this be the default behaviour since infer_json_schema does it by default?

Yes, confirmed that if you mismatch numbers and bools, it will also consider them to be strings.

since infer_json_schema does it by default

I think having an option that disables strict enforcement along the lines of the schema inference makes sense 👍, I do think it should still be optional as some workflows care about schema consistency and would rather fail loud 😄

Then I'll keep it very simple. Just a bool in a struct for now.

tustvold · 2023-02-20T11:56:56Z

arrow-json/src/raw/converter.rs

+    }
+}
+
+fn obj_to_json_string<'a, C: TapeConverter>(


I personally think this behaviour is a little bit strange, I think it should be possible to opt-in to coercing primitives, e.g. string <-> bool, separately from opting into this potentially more surprising behaviour?

rguerreiromsft · 2023-02-20T17:37:38Z

I do also wonder if we really need generics for this, or if a simple strict flag is all that is needed....

I was thinking about this when implementing too. I tried to make it a Box before but it didn't like that to_primitive is generic. So it couldn't be object safe.

I was going to offer another PR with just the flag. Instead I'll rework this one and squash everything into one commit at the end. I agree with you that leaking implementation details makes it harder to evolve without breaking changes.

I can create a few different flags that the user can opt-in:

pub enum DecoderParserStrategy {
    PrimitiveToString, // Accepts 1 as "1" and true as "true"
    ObjectToJsonString, // Accepts [123] as "[123]" and {"a": 123} as "{\"a\": 123}"
    NumberToBool, // Accepts 0 as false, otherwise it's true
    BoolToNumber, // true becomes 1 and false becomes 0
}

Then the user can provide these flags using an array, and by default none of them are enabled.

I'll also make sure it can parse \n and " from a string, maybe the best option is to use serde_json to do that.

tustvold · 2023-02-20T17:43:08Z

Then the user can provide these flags using an array, and by default none of them are enabled.

I would recommend a struct of booleans, as opposed to an array, given these will need to be checked on the "hot path". I would also recommend keeping it simple, with a single coerce_primitives flag or something... I think complex schema coercion logic is possibly better handled at a higher level...

rguerreiromsft · 2023-02-20T19:12:42Z

@tustvold, thank you so much for all the guidance. I have updated this PR with the suggested changes.

…sense than the Tape

tustvold

Looks good to me, thank you. I will double-check the benchmarks tomorrow prior to getting this in

tustvold · 2023-02-21T14:00:25Z

I've confirmed this does not seem to have a discernible impact on the benchmarks 🎉 Thank you for sticking with this

ursabot · 2023-02-21T14:01:59Z

Benchmark runs are scheduled for baseline = 25da74c and contender = e33dbe2. e33dbe2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added the arrow Changes to the arrow crate label Feb 20, 2023

tustvold reviewed Feb 20, 2023

View reviewed changes

Add ability to coerce primitive values into string behind an option

59cd225

rguerreiromsft force-pushed the convert-json branch from 77213bd to 59cd225 Compare February 20, 2023 19:10

Moving coerce_primitive flag into the Decoders because it makes more …

355f030

…sense than the Tape

tustvold approved these changes Feb 20, 2023

View reviewed changes

tustvold changed the title ~~Add converter to allow custom parsing of json data~~ Support String Coercion in Raw JSON Reader Feb 21, 2023

tustvold merged commit e33dbe2 into apache:master Feb 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support String Coercion in Raw JSON Reader #3736

Support String Coercion in Raw JSON Reader #3736

rguerreiromsft commented Feb 20, 2023 •

edited

Loading

rguerreiromsft commented Feb 20, 2023 •

edited

Loading

tustvold left a comment

tustvold Feb 20, 2023

tustvold Feb 20, 2023

rguerreiromsft Feb 20, 2023

tustvold Feb 20, 2023

rguerreiromsft Feb 20, 2023

tustvold Feb 20, 2023 •

edited

Loading

rguerreiromsft Feb 20, 2023

rguerreiromsft Feb 20, 2023

tustvold Feb 20, 2023

rguerreiromsft Feb 20, 2023 •

edited

Loading

tustvold Feb 20, 2023

rguerreiromsft commented Feb 20, 2023 •

edited

Loading

tustvold commented Feb 20, 2023 •

edited

Loading

rguerreiromsft commented Feb 20, 2023

tustvold left a comment

tustvold commented Feb 21, 2023 •

edited

Loading

ursabot commented Feb 21, 2023

Support String Coercion in Raw JSON Reader #3736

Support String Coercion in Raw JSON Reader #3736

Conversation

rguerreiromsft commented Feb 20, 2023 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

rguerreiromsft commented Feb 20, 2023 • edited Loading

tustvold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rguerreiromsft Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rguerreiromsft commented Feb 20, 2023 • edited Loading

tustvold commented Feb 20, 2023 • edited Loading

rguerreiromsft commented Feb 20, 2023

tustvold left a comment

Choose a reason for hiding this comment

tustvold commented Feb 21, 2023 • edited Loading

ursabot commented Feb 21, 2023

rguerreiromsft commented Feb 20, 2023 •

edited

Loading

rguerreiromsft commented Feb 20, 2023 •

edited

Loading

tustvold Feb 20, 2023 •

edited

Loading

rguerreiromsft Feb 20, 2023 •

edited

Loading

rguerreiromsft commented Feb 20, 2023 •

edited

Loading

tustvold commented Feb 20, 2023 •

edited

Loading

tustvold commented Feb 21, 2023 •

edited

Loading