You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "scovich (via GitHub)" <gi...@apache.org> on 2026/01/26 22:42:29 UTC
[PR] [WIP] Json decoder factory v2 [arrow-rs]
scovich opened a new pull request, #9272:
URL: https://github.com/apache/arrow-rs/pull/9272
# Which issue does this PR close?
This is a variant of https://github.com/apache/arrow-rs/pull/9259, but stacked on top of three building block PR:
* https://github.com/apache/arrow-rs/pull/9266
* https://github.com/apache/arrow-rs/pull/9270
* https://github.com/apache/arrow-rs/pull/9271
- Part of https://github.com/apache/arrow-rs/issues/8987
# Rationale for this change
See description of https://github.com/apache/arrow-rs/pull/9259. This version here factors out building blocks so it's easier to see what _actually_ changes to add custom json decoder support.
# What changes are included in this PR?
See description of https://github.com/apache/arrow-rs/pull/9259. Same net change, just organized differently.
# Are these changes tested?
Yes. Existing and newly added unit tests.
# Are there any user-facing changes?
* Make JSON tape decoder classes public
* Make JSON `ArrayDecoder` trait public
* Make JSON `DecoderContext` class public
* New public trait: `DecoderFactory`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Json decoder factory v2 [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #9272:
URL: https://github.com/apache/arrow-rs/pull/9272#issuecomment-3808207352
Ok, rebased now that the three prefactor PR merged. The diff looks big, but the bulk of it is tests and doc comments. The actual change is pretty small.
The biggest single contributors are:
* 800+ LoC for the various motivating examples/tests in custom_decoder_tests.rs
* Hopefully they're easily understood and help spur a healthy discussion on what the API should actually look like
* TBD whether they stay as tests for now, or if some are deemed useful enough to become part of the API
* ~280 LoC to define the variant decoder and factory (+ tests)
* Could potentially split out to a separate PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Json decoder factory v2 [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #9272:
URL: https://github.com/apache/arrow-rs/pull/9272#discussion_r2742894183
##########
arrow-json/src/lib.rs:
##########
@@ -86,7 +86,7 @@
pub mod reader;
pub mod writer;
-pub use self::reader::{Reader, ReaderBuilder};
+pub use self::reader::{ArrayDecoder, DecoderFactory, Reader, ReaderBuilder, Tape, TapeElement};
Review Comment:
There were some discussions a while back about the way that it copies all strings from the source data being unfortunate. I'm not sure how this is avoidable with the push-based decoder interface, but it has been discussed.
TBC I am not against making Tape public, but it likely needs some TLC prior to that to ensure it is usable and vaguely future-proof. Even basic things like adding non_exhaustive, etc...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Json decoder factory v2 [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9272:
URL: https://github.com/apache/arrow-rs/pull/9272#discussion_r2800410816
##########
arrow-json/src/lib.rs:
##########
@@ -86,7 +86,7 @@
pub mod reader;
pub mod writer;
-pub use self::reader::{Reader, ReaderBuilder};
+pub use self::reader::{ArrayDecoder, DecoderFactory, Reader, ReaderBuilder, Tape, TapeElement};
Review Comment:
I won't have time in the next couple of weeks, sadly. But yes I do want to see this through.
I do tend to agree we should get something out once it's reasonable, but we already know of several use cases and it's not obvious to me that even those known use cases are well-served by the current approach (speaking as somebody who wants to actually _use_ whatever we come up with).
So: If we have something that works well for the use cases we've thought of, it's probably Good Enough and can evolve a quarter or two later as we discover more use cases or warts.
Is that a reasonable go/no-go criteria?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Json decoder factory v2 [arrow-rs]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9272:
URL: https://github.com/apache/arrow-rs/pull/9272#issuecomment-4795285833
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Json decoder factory v2 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9272:
URL: https://github.com/apache/arrow-rs/pull/9272#discussion_r2801106651
##########
arrow-json/src/lib.rs:
##########
@@ -86,7 +86,7 @@
pub mod reader;
pub mod writer;
-pub use self::reader::{Reader, ReaderBuilder};
+pub use self::reader::{ArrayDecoder, DecoderFactory, Reader, ReaderBuilder, Tape, TapeElement};
Review Comment:
> So: If we have something that works well for the use cases we've thought of, it's probably Good Enough and can evolve a quarter or two later as we discover more use cases or warts.
This is my personal preferred approach -- let's get it out rather than waiting on something that is perfect
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Json decoder factory v2 [arrow-rs]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #9272: [WIP] Json decoder factory v2
URL: https://github.com/apache/arrow-rs/pull/9272
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Json decoder factory v2 [arrow-rs]
Posted by "debugmiller (via GitHub)" <gi...@apache.org>.
debugmiller commented on code in PR #9272:
URL: https://github.com/apache/arrow-rs/pull/9272#discussion_r2777467199
##########
arrow-json/src/lib.rs:
##########
@@ -86,7 +86,7 @@
pub mod reader;
pub mod writer;
-pub use self::reader::{Reader, ReaderBuilder};
+pub use self::reader::{ArrayDecoder, DecoderFactory, Reader, ReaderBuilder, Tape, TapeElement};
Review Comment:
If someone could come up with a list of changes that would make us comfortable making Tape public, I can try to put up a PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Json decoder factory v2 [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #9272:
URL: https://github.com/apache/arrow-rs/pull/9272#discussion_r2742894183
##########
arrow-json/src/lib.rs:
##########
@@ -86,7 +86,7 @@
pub mod reader;
pub mod writer;
-pub use self::reader::{Reader, ReaderBuilder};
+pub use self::reader::{ArrayDecoder, DecoderFactory, Reader, ReaderBuilder, Tape, TapeElement};
Review Comment:
There were some discussions a while back about the way that it copies all strings from the source data being unfortunate. I'm not sure how this is avoidable with the push-based decoder interface, but it has been discussed.
TBC I am not against making Tape public, but it likely needs some TLC prior to that to ensure it is usable and vaguely future-proof. Even basic things like adding non_exhaustive, hiding methods like [this](https://github.com/apache/arrow-rs/blob/7e5076f1f775a6fd08a4d63389e26e2920fe3f6a/arrow-json/src/reader/tape.rs#L209) that are a bit odd to expose, etc...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Json decoder factory v2 [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #9272:
URL: https://github.com/apache/arrow-rs/pull/9272#issuecomment-3802111316
Oops @alamb this should prob stay in draft status until (a) the dependencies it's stacked on merge; and (b) we decide we actually _want_ this approach in this form?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Json decoder factory v2 [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #9272:
URL: https://github.com/apache/arrow-rs/pull/9272#discussion_r2735647557
##########
arrow-json/src/lib.rs:
##########
@@ -86,7 +86,7 @@
pub mod reader;
pub mod writer;
-pub use self::reader::{Reader, ReaderBuilder};
+pub use self::reader::{ArrayDecoder, DecoderFactory, Reader, ReaderBuilder, Tape, TapeElement};
Review Comment:
I think this is the key part that might be deemed controversial. Is Tape really a good thing to expose publicly? It's been a while since I wrote it, but I remember it not being especially friendly as an API, and something that stands a good chance of being changed in future - e.g. to avoid copying strings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Json decoder factory v2 [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #9272:
URL: https://github.com/apache/arrow-rs/pull/9272#issuecomment-3848273734
I just encountered (+ remembered) another use case for custom JSON decoding: Wrong-named fields. Whether due to schema evolution or flat-out typos, it may be desirable for multiple input field names to map to the same output field (with some precedence rules if multiple candidates are available).
Just lodging that thought here for now -- I didn't have time to see if the approach explored here can actually handle the use case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Json decoder factory v2 [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9272:
URL: https://github.com/apache/arrow-rs/pull/9272#discussion_r2736708169
##########
arrow-json/src/lib.rs:
##########
@@ -86,7 +86,7 @@
pub mod reader;
pub mod writer;
-pub use self::reader::{Reader, ReaderBuilder};
+pub use self::reader::{ArrayDecoder, DecoderFactory, Reader, ReaderBuilder, Tape, TapeElement};
Review Comment:
Good question, and I don't remember seeing any discussion on the original PR I'm building on here:
* https://github.com/apache/arrow-rs/pull/9021
Is there any way to allow users to customize parsing without exposing _something_? Other options might include:
1. Create a new trait or wrapper that exposes the tape's information in a simplified/safe/stable way, to decouple users from the low-level details.
* Maybe could work? Worth exploring?
3. Convert the tape to variant, and shift the factory/decoder stuff over to variant-compute instead of json crate
* We'd still need _something_ to allow parsing JSON to variant, which I believe is a canonical extension types that _should_ be supported directly.
* Variant is insanely complex once shredding comes into the picture, so such an interface would not be easier or safer to use IMO.
* The extra layer of conversion would impose significant overhead for somebody who just wants to parse a few misbehaving columns in a special way.
4. Something else I'm not thinking of?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Json decoder factory v2 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9272:
URL: https://github.com/apache/arrow-rs/pull/9272#discussion_r2738516198
##########
arrow-json/src/lib.rs:
##########
@@ -86,7 +86,7 @@
pub mod reader;
pub mod writer;
-pub use self::reader::{Reader, ReaderBuilder};
+pub use self::reader::{ArrayDecoder, DecoderFactory, Reader, ReaderBuilder, Tape, TapeElement};
Review Comment:
I agree if we want to allow users to override decoding behavior we are going to have to given them direct access to Tape / Tape Element - I don't really see any way around it
> something that stands a good chance of being changed in future - e.g. to avoid copying strings.
@tustvold -- what strings are you referring to? I don't see any strings copied here:
https://github.com/apache/arrow-rs/blob/7e5076f1f775a6fd08a4d63389e26e2920fe3f6a/arrow-json/src/reader/tape.rs#L34-L33
https://github.com/apache/arrow-rs/blob/7e5076f1f775a6fd08a4d63389e26e2920fe3f6a/arrow-json/src/reader/tape.rs#L96-L101
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WIP] Json decoder factory v2 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9272:
URL: https://github.com/apache/arrow-rs/pull/9272#discussion_r2795198849
##########
arrow-json/src/lib.rs:
##########
@@ -86,7 +86,7 @@
pub mod reader;
pub mod writer;
-pub use self::reader::{Reader, ReaderBuilder};
+pub use self::reader::{ArrayDecoder, DecoderFactory, Reader, ReaderBuilder, Tape, TapeElement};
Review Comment:
@scovich is this something you can help drive? I think it is important but I really just don't have the bandwidth to give it the attention it deserves
From my perspective, the Arrow crates now permit making breaking API changes every 3 months (major releases) so if we expose a structure and then decide to make breaking changes to it, that isn't impossible
Thus in my mind, I think we should get something out and working
I don't suspect we are not likely to see changes to this interface unless we expose it and there are new use cases put forward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org