Add hooks to json encoder to override default encoding or add support for unsupported types #7015

adriangb · 2025-01-24T12:37:19Z

We essentially had to vendor this module to implement this. I know this has been discussed in the past and there was pushback against exposing Encoder to the public API but since I had to go through the work of updating our vendored version I thought I'd at least try to make a PR to upstream this to make our lives easier in the future and bring value back to the community.

In my opinion exposing this is a rather small adition the public API and any breaking changes in the future will be restricted to the JSON encoder, which is not a core part of the project. It seems worth it and addresses various requests e.g. how to encode a binary array (base64 encoded string or array of ints are both common). It has also been a stable API for ~ 1 year now.

… for unsupported types

adriangb · 2025-01-24T12:37:34Z

TODO: add a test showing how to change the encoding of binary arrays.

adriangb · 2025-01-24T12:37:55Z

cc @tustvold

adriangb · 2025-01-24T15:15:18Z

TODO: add a test showing how to change the encoding of binary arrays.

Done

adriangb · 2025-01-24T15:15:56Z

arrow-json/src/writer/encoder.rs

+type EncoderFactoryResult<'a> =
+    Result<Option<(Box<dyn Encoder + 'a>, Option<NullBuffer>)>, ArrowError>;


Should this be Option<&'a NullBuffer>?

It probably could be for consistency, but it isn't likely to have any material performance impact

tustvold

I had a review, and think it is probably fine to expose Encoder, it is a relatively straightforward trait that I don't see changing. It is the reader side that I'd be much more hesitant about altering, as that is quite coupled with the parsing machinery.

That being said there is one slight wrinkle, as documented here

The implementation of Encoder for ArrayFormatter assumes it does not produce
characters that would need to be escaped within a JSON string, e.g. '"'.
If support for user-provided format specifications is added, this assumption
may need to be revisited

I think we need to do the following:

Remove the impl of Encoder for ArrayFormatter in favour of a private newtype wrapper
Clearly document on Encoder the expectation that it generates valid escaped JSON

On a related note, the return type of Box<dyn Encoder + 'a>, Option<NullBuffer> is a bit of a mouthful, and honestly makes for a bit of a funky API where the semantics of an Encoder depend on a different data structure returned at construction time.

I wonder if a better API might be something like

pub trait Encoder {
    fn encode(&mut self, idx: usize, out: &mut Vec<u8>);
    fn nulls(&self) -> Option<NullBuffer>;
}

I also left a comment on potentially simplifying the factory down to a single method

tustvold · 2025-01-25T15:28:43Z

arrow-json/src/writer/encoder.rs

+        _array: &'a dyn Array,
+        _data_type: &DataType,


Why do we pass both DataType and Array?

We don't need to, it just simplifies the implementation of the EncoderFactory a bit because almost surely all of them are going to call array.data_type(). I can remove if you'd prefer.

tustvold · 2025-01-25T15:29:05Z

arrow-json/src/writer/mod.rs

@@ -426,10 +438,13 @@ mod tests {

        let actual: Vec<Option<Value>> = input
            .split(|b| *b == b'\n')
-            .map(|s| (!s.is_empty()).then(|| serde_json::from_slice(s).unwrap()))
+            .map(|s| {
+                println!("{:?}", str::from_utf8(s));


tustvold · 2025-01-25T15:29:57Z

arrow-json/src/writer/mod.rs

+                    }
+                }
+                let (_, type_ids, _, buffers) = array
+                    .as_any()


Can we use AsArray instead of manual downcasts, it is much easier to read

tustvold · 2025-01-25T15:45:05Z

arrow-json/src/writer/encoder.rs

+
+    /// Make an encoder that if returned runs after all of the default encoders.
+    /// If this method returns None then an error will be returned.
+    fn make_fallback_encoder<'a>(


Why do we need this? AFAICT we could remove this with no real loss of functionality, if anything it would be more resilient as adding a new default encoder wouldn't break existing overrides.

I'll drop it in favor of a single method, I think it should work for our use case.

tustvold · 2025-01-25T15:47:36Z

arrow-json/src/writer/mod.rs

+        }
+
+        #[derive(Debug)]
+        struct UnionEncoder {


FWIW adding a native UnionEncoder is likely not overly complicated

That would be nice 😄

How about I try to write one on our end and then we can upstream it if it works properly?

tustvold · 2025-01-25T15:49:48Z

arrow-json/src/writer/encoder.rs

+type EncoderFactoryResult<'a> =
+    Result<Option<(Box<dyn Encoder + 'a>, Option<NullBuffer>)>, ArrowError>;


It probably could be for consistency, but it isn't likely to have any material performance impact

adriangb · 2025-01-25T19:49:54Z

Thank you for the review. I've pushed a (broken) version with most comments addressed aside from the part about ArrayFormater. To be honest I don't understand what ArrayFormatter does or why it's special... would you mind elaborating a bit on how you propose to handle it?

tustvold · 2025-01-25T20:26:19Z

ArrayFormatter is a type from arrow_cast used to format array values to strings. It is used by arrow_json to format some values, however, being a general formatter it does not implement JSON escaping. This isn't a problem for the types arrow_json uses it for, but as we're now making Encoder public we need to ensure people don't think ArrayFormatter can be used as an Encoder in the general case

adriangb · 2025-01-26T16:31:05Z

Okay I think I understand now. Looking at the implementation it seems to me that it wouldn't be that much extra work to add formatters for Date, Timestamp, Time and Decimal, then we could ditch ArrayFormatter for JSON encoding altogether. I can do that as a prequel PR. Wdyt?

tustvold · 2025-01-26T16:53:44Z

A private newtype wrapper would avoid a non-trivial amount of code duplication whilst resolving the issue, and would be my preferred approach

adriangb · 2025-01-26T17:42:20Z

Makes sense, done!

There is quite a bit of code churn from changing the Encoder trait. Would you like me to break that out into a precursor PR?

add hooks to json encoder to override default encoding or add support…

c566e88

… for unsupported types

github-actions bot added the arrow Changes to the arrow crate label Jan 24, 2025

adriangb added 3 commits January 24, 2025 13:29

fmt

a318bad

add test

81af1a5

better test

3a474f3

adriangb commented Jan 24, 2025

View reviewed changes

tustvold reviewed Jan 25, 2025

View reviewed changes

adriangb added 4 commits January 25, 2025 19:00

remove fallback_encoder and data_type

d381316

address other small bits of feedback

bf1fd18

wip on suggested refactor

8905c7e

remove type

80a2cbf

adriangb added 2 commits January 26, 2025 09:37

wrap ArrayFormatter

4f9c622

fix

e2231ce

lint

a21bbc7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hooks to json encoder to override default encoding or add support for unsupported types #7015

Add hooks to json encoder to override default encoding or add support for unsupported types #7015

adriangb commented Jan 24, 2025

adriangb commented Jan 24, 2025

adriangb commented Jan 24, 2025

adriangb commented Jan 24, 2025

adriangb Jan 24, 2025

tustvold Jan 25, 2025

tustvold left a comment

tustvold Jan 25, 2025

adriangb Jan 25, 2025

tustvold Jan 25, 2025

adriangb Jan 25, 2025

tustvold Jan 25, 2025

tustvold Jan 25, 2025

adriangb Jan 25, 2025

tustvold Jan 25, 2025

adriangb Jan 25, 2025

tustvold Jan 25, 2025

adriangb commented Jan 25, 2025

tustvold commented Jan 25, 2025

adriangb commented Jan 26, 2025

tustvold commented Jan 26, 2025

adriangb commented Jan 26, 2025

		type EncoderFactoryResult<'a> =
		Result<Option<(Box<dyn Encoder + 'a>, Option<NullBuffer>)>, ArrowError>;

Add hooks to json encoder to override default encoding or add support for unsupported types #7015

Are you sure you want to change the base?

Add hooks to json encoder to override default encoding or add support for unsupported types #7015

Conversation

adriangb commented Jan 24, 2025

adriangb commented Jan 24, 2025

adriangb commented Jan 24, 2025

adriangb commented Jan 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

adriangb commented Jan 25, 2025

tustvold commented Jan 25, 2025

adriangb commented Jan 26, 2025

tustvold commented Jan 26, 2025

adriangb commented Jan 26, 2025