feat: encode FixedSizeBinary in JSON as hex string #5622

hiltontj · 2024-04-10T02:02:04Z

Which issue does this PR close?

Closes #5620.

Rationale for this change

See #5620.

What changes are included in this PR?

Adds encoding support to the JSON writer for the FixedSizeBinary data type.

A simple test was added as well. I can add more extensive testing if needed.

Are there any user-facing changes?

Potentially some documentation to state that FixedSizeBinary is encoded as hex strings.

Adds encoding support to the JSON writer for the FixedSizeBinary DataType A test was added as well

tustvold · 2024-04-10T08:05:58Z

arrow-json/Cargo.toml

@@ -56,6 +56,7 @@ tokio = { version = "1.27", default-features = false, features = ["io-util"] }
 bytes = "1.4"
 criterion = { version = "0.5", default-features = false }
 rand = { version = "0.8", default-features = false, features = ["std", "std_rng"] }
+hex = "0.4.3"


Do we need this?

No, it is not required. The reason I added it was so that in the test I added, it was apparent that the bytes in the column resulted in the correct HEX string. I will see if I can write the test in a way that gets by without it.

I updated the test so that it no longer relies on the hex crate, and removed the dep. I think it would be nice to have a test that fully round-trips data, first by decoding from HEX to the FixedSizeBinary, then encoding back. But, I am not sure if we want to support this behaviour on the JSON reader, and this issue is only about the write side.

hiltontj · 2024-04-10T13:02:43Z

I realize now, that the test I wrote is complete bogus (not valid JSON). So I am reworking that.

The fixed size binary values were not being encoded with surrounding double quotes. This fixes that, and updates the added test to actually parse the written JSON as JSON, using serde_json, and make assertions against that.

alamb

Thanks @hiltontj ! This is looking close -- I am not sure it handles nulls properly yet, but I think that should be straightforward to add

arrow-json/src/writer.rs

alamb · 2024-04-10T14:00:48Z

arrow-json/src/writer/encoder.rs

+
+impl Encoder for FixedSizeBinaryEncoder {
+    fn encode(&mut self, idx: usize, out: &mut Vec<u8>) {
+        let v = self.0.value(idx);


I think this needs to check is_valid before serializing (I think you'll see the need when testing for null)

Have the FixedSizeBinaryEncoder for the JSON writer handle explicit null values, based on the Writer's EncoderOptions.

hiltontj · 2024-04-10T15:07:32Z

@alamb - I think I addressed your comments. With 35371d3, explicit nulls were actually being handled correctly in the test, but I believe this was because of the wrapping list/map encoders. So, I extended the explicit null support down to the FixedSizeBinaryEncoder in 9768a43 directly, to be sure.

alamb

Looks great to me -- thank you @hiltontj for adding the new tests

arrow-json/src/writer.rs

alamb · 2024-04-10T16:43:15Z

I think this is good to go -- any concerns @tustvold ?

Changed the FixedSizeBinaryEncoder for the JSON writer to use a borrow of the FixedSizeBinaryArray being encoded, to follow other Encoder implementations, and to remove the use of clone.

hiltontj · 2024-04-12T00:42:59Z

@alamb - I changed the FixedSizeBinaryEncoder to use a borrow of the FixedSizeBinaryArray, instead of cloning it.

BooleanEncoder and StringEncoder were changed to use borrows of their respective Array types, to avoid cloning.

hiltontj · 2024-04-12T00:51:01Z

I also noticed that the StringEncoder and BooleanEncoder were needlessly cloning, so I updated them to use borrows, and removed the .clone()s. Hope you don't mind.

tustvold · 2024-04-12T08:10:47Z

arrow-json/src/writer/encoder.rs

+
+impl<'a> Encoder for FixedSizeBinaryEncoder<'a> {
+    fn encode(&mut self, idx: usize, out: &mut Vec<u8>) {
+        if self.array.is_valid(idx) {


IIRC nullability is handled by the parent and so this shouldn't be necessary for a non nested type

I double checked by making the following change locally and the tests all still pass. Thus I agree the valid check is not needed

diff --git a/arrow-json/src/writer/encoder.rs b/arrow-json/src/writer/encoder.rs index 27fa5747288..470d2fadd06 100644 --- a/arrow-json/src/writer/encoder.rs +++ b/arrow-json/src/writer/encoder.rs @@ -101,7 +101,7 @@ fn make_encoder_impl<'a>( DataType::FixedSizeBinary(_) => { let array = array.as_any().downcast_ref::<FixedSizeBinaryArray>().unwrap(); - (Box::new(FixedSizeBinaryEncoder::new(array, options)) as _, array.nulls().cloned()) + (Box::new(FixedSizeBinaryEncoder::new(array)) as _, array.nulls().cloned()) } DataType::Struct(fields) => { @@ -451,14 +451,12 @@ impl<'a> Encoder for MapEncoder<'a> { struct FixedSizeBinaryEncoder<'a> { array: &'a FixedSizeBinaryArray, - explicit_nulls: bool, } impl<'a> FixedSizeBinaryEncoder<'a> { - fn new(array: &'a FixedSizeBinaryArray, options: &EncoderOptions) -> Self { + fn new(array: &'a FixedSizeBinaryArray) -> Self { Self { array, - explicit_nulls: options.explicit_nulls, } } } @@ -473,8 +471,6 @@ impl<'a> Encoder for FixedSizeBinaryEncoder<'a> { write!(out, "{byte:02x}").unwrap(); } out.push(b'"'); - } else if self.explicit_nulls { - out.extend_from_slice(b"null"); } } }

Indeed, as per #5622 (comment), when I first added the test, it was passing without adding the explicit_nulls option to the FixedSizeBinaryEncoder, but I added it anyways. I am totally fine with taking it out.

tustvold · 2024-04-12T08:15:27Z

were needlessly cloning

A couple of reference counts are irrelevant compared to encoding overheads, but I suppose it can't hurt to avoid them

alamb

Thanks @hiltontj -- I think this PR could be merged as is.

@tustvold has a good suggestion for simplificiation https://github.com/apache/arrow-rs/pull/5622/files#r1562191540 but I also think we could do that as a follow on PR if you prefer

Just let me know what you would prefer

alamb · 2024-04-12T12:31:01Z

arrow-json/src/writer/encoder.rs

+
+impl<'a> Encoder for FixedSizeBinaryEncoder<'a> {
+    fn encode(&mut self, idx: usize, out: &mut Vec<u8>) {
+        if self.array.is_valid(idx) {


I double checked by making the following change locally and the tests all still pass. Thus I agree the valid check is not needed

diff --git a/arrow-json/src/writer/encoder.rs b/arrow-json/src/writer/encoder.rs index 27fa5747288..470d2fadd06 100644 --- a/arrow-json/src/writer/encoder.rs +++ b/arrow-json/src/writer/encoder.rs @@ -101,7 +101,7 @@ fn make_encoder_impl<'a>( DataType::FixedSizeBinary(_) => { let array = array.as_any().downcast_ref::<FixedSizeBinaryArray>().unwrap(); - (Box::new(FixedSizeBinaryEncoder::new(array, options)) as _, array.nulls().cloned()) + (Box::new(FixedSizeBinaryEncoder::new(array)) as _, array.nulls().cloned()) } DataType::Struct(fields) => { @@ -451,14 +451,12 @@ impl<'a> Encoder for MapEncoder<'a> { struct FixedSizeBinaryEncoder<'a> { array: &'a FixedSizeBinaryArray, - explicit_nulls: bool, } impl<'a> FixedSizeBinaryEncoder<'a> { - fn new(array: &'a FixedSizeBinaryArray, options: &EncoderOptions) -> Self { + fn new(array: &'a FixedSizeBinaryArray) -> Self { Self { array, - explicit_nulls: options.explicit_nulls, } } } @@ -473,8 +471,6 @@ impl<'a> Encoder for FixedSizeBinaryEncoder<'a> { write!(out, "{byte:02x}").unwrap(); } out.push(b'"'); - } else if self.explicit_nulls { - out.extend_from_slice(b"null"); } } }

hiltontj · 2024-04-12T12:42:12Z

@alamb - I will update as suggested on this PR and re-request review, no problem!

The FixedSizeBinaryEncoder does not need to handle nulls, as that will be handled by a parent encoder, i.e., list/map.

alamb

Thanks @hiltontj for sticking with this. It looks very nice at the end 🙏

feat: encode FixedSizeBinary in JSON as hex

1b86dfd

Adds encoding support to the JSON writer for the FixedSizeBinary DataType A test was added as well

github-actions bot added the arrow Changes to the arrow crate label Apr 10, 2024

tustvold reviewed Apr 10, 2024

View reviewed changes

hiltontj added 2 commits April 10, 2024 09:15

fix: properly encode fixed size binary as string

89ffe1a

The fixed size binary values were not being encoded with surrounding double quotes. This fixes that, and updates the added test to actually parse the written JSON as JSON, using serde_json, and make assertions against that.

chore: remove unused hex dep in arrow-json

440c7cc

alamb reviewed Apr 10, 2024

View reviewed changes

alamb mentioned this pull request Apr 10, 2024

DataFusion weekly project plan (Andrew Lamb) - April 8, 2024 apache/datafusion#10002

Closed

9 tasks

hiltontj added 2 commits April 10, 2024 10:39

refactor: check for null serialization of fixedsizebinary in JSON

35371d3

refactor: extend explicit nulls to the FixedSizeBinaryEncoder

9768a43

Have the FixedSizeBinaryEncoder for the JSON writer handle explicit null values, based on the Writer's EncoderOptions.

alamb approved these changes Apr 10, 2024

View reviewed changes

arrow-json/src/writer.rs Show resolved Hide resolved

refactor: borrow array in JSON fixed size binary encoder

94e6396

Changed the FixedSizeBinaryEncoder for the JSON writer to use a borrow of the FixedSizeBinaryArray being encoded, to follow other Encoder implementations, and to remove the use of clone.

refactor: remove need for clone in JSON encoder types

407dbea

BooleanEncoder and StringEncoder were changed to use borrows of their respective Array types, to avoid cloning.

hiltontj requested a review from alamb April 12, 2024 00:51

tustvold reviewed Apr 12, 2024

View reviewed changes

alamb approved these changes Apr 12, 2024

View reviewed changes

refactor: remove null handling in JSON FixedSizeBinaryEncoder

c66e24c

The FixedSizeBinaryEncoder does not need to handle nulls, as that will be handled by a parent encoder, i.e., list/map.

hiltontj requested a review from alamb April 12, 2024 12:46

alamb approved these changes Apr 12, 2024

View reviewed changes

alamb merged commit 6450527 into apache:master Apr 12, 2024
22 checks passed

hiltontj deleted the json-fixed-size-binary branch April 12, 2024 20:01

tustvold mentioned this pull request Apr 17, 2024

Serialize FixedSizeBinary as HEX with JSON Writer #5620

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: encode FixedSizeBinary in JSON as hex string #5622

feat: encode FixedSizeBinary in JSON as hex string #5622

hiltontj commented Apr 10, 2024

tustvold Apr 10, 2024

hiltontj Apr 10, 2024

hiltontj Apr 10, 2024

hiltontj commented Apr 10, 2024

alamb left a comment

alamb Apr 10, 2024

hiltontj commented Apr 10, 2024

alamb left a comment

alamb commented Apr 10, 2024

hiltontj commented Apr 12, 2024

hiltontj commented Apr 12, 2024

tustvold Apr 12, 2024 •

edited

Loading

alamb Apr 12, 2024 •

edited

Loading

hiltontj Apr 12, 2024

tustvold commented Apr 12, 2024

alamb left a comment

alamb Apr 12, 2024 •

edited

Loading

hiltontj commented Apr 12, 2024

alamb left a comment

feat: encode FixedSizeBinary in JSON as hex string #5622

feat: encode FixedSizeBinary in JSON as hex string #5622

Conversation

hiltontj commented Apr 10, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold Apr 10, 2024

Choose a reason for hiding this comment

hiltontj Apr 10, 2024

Choose a reason for hiding this comment

hiltontj Apr 10, 2024

Choose a reason for hiding this comment

hiltontj commented Apr 10, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Apr 10, 2024

Choose a reason for hiding this comment

hiltontj commented Apr 10, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb commented Apr 10, 2024

hiltontj commented Apr 12, 2024

hiltontj commented Apr 12, 2024

tustvold Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

alamb Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

hiltontj Apr 12, 2024

Choose a reason for hiding this comment

tustvold commented Apr 12, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

hiltontj commented Apr 12, 2024

alamb left a comment

Choose a reason for hiding this comment

tustvold Apr 12, 2024 •

edited

Loading

alamb Apr 12, 2024 •

edited

Loading

alamb Apr 12, 2024 •

edited

Loading