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 19:53:06 UTC
[PR] make_decoder accepts borrowed DataType instead of owned [arrow-rs]
scovich opened a new pull request, #9270:
URL: https://github.com/apache/arrow-rs/pull/9270
# Which issue does this PR close?
- Part of https://github.com/apache/arrow-rs/issues/8987
# Rationale for this change
Today's json decoder helper, `make_decoder`, takes an owned data type whose components are cloned at every level during the recursive decoder initialization process. This breaks pointer stability of the resulting `DataType` instances that a custom JSON decoder factory would see, vs. those of the schema it and the reader builder were initialized with.
The lack of pointer stability prevents users from creating "path based" decoder factories, that are able to customize decoder behavior based not only on type, but also on the field's path in the schema. See the `PathBasedDecoderFactory` in arrow-json/tests/custom_decoder_tests.rs of https://github.com/apache/arrow-rs/pull/9259, for an example.
# What changes are included in this PR?
By passing `&DataType` instead, we change code like this:
```rust
let decoder = make_decoder(field.data_type().clone(), ...);
Ok(Self {
data_type,
decoder,
...
})
```
to this:
```rust
let child_decoder = make_decoder(field.data_type(), ...);
Ok(Self {
data_type: data_type.clone(),
decoder,
...
})
```
Result: Every call to `make_decoder` receives a reference to the actual original data type from the builder's input schema. The final decoder `Self` is unchanged -- it already received a clone and continues to do so.
NOTE: There is one additional clone of the top-level `DataType::Struct` we create for normal (`!is_field`) builders. But that's a cheap arc clone of a `Fields` member.
# Are these changes tested?
Yes, existing unit tests validate the change.
# Are there any user-facing changes?
No. All functions and data types involved are private -- the array decoders are marked `pub` but are defined in a private mod with no public re-export that would make them available outside the crate.
--
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] make_decoder accepts borrowed DataType instead of owned [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9270:
URL: https://github.com/apache/arrow-rs/pull/9270#discussion_r2729535970
##########
arrow-json/src/reader/struct_array.rs:
##########
@@ -81,28 +81,30 @@ pub struct StructArrayDecoder {
impl StructArrayDecoder {
pub fn new(
ctx: &DecoderContext,
- data_type: DataType,
+ data_type: &DataType,
is_nullable: bool,
) -> Result<Self, ArrowError> {
- let struct_mode = ctx.struct_mode();
- let fields = struct_fields(&data_type);
-
- // If this struct nullable, need to permit nullability in child array
- // StructArrayDecoder::decode verifies that if the child is not nullable
- // it doesn't contain any nulls not masked by its parent
Review Comment:
thank you
--
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] make_decoder accepts borrowed DataType instead of owned [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #9270:
URL: https://github.com/apache/arrow-rs/pull/9270
--
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] make_decoder accepts borrowed DataType instead of owned [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9270:
URL: https://github.com/apache/arrow-rs/pull/9270#issuecomment-3802035177
(this PR needs to have some conflicts resolved)
--
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] make_decoder accepts borrowed DataType instead of owned [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #9270:
URL: https://github.com/apache/arrow-rs/pull/9270#issuecomment-3802409767
@alamb -- Is it normal for MIRI to take O(hours)? It's been stuck here for a very long time:
```
...
test rank::tests::test_bytes ... ok
test rank::tests::test_get_boolean_rank_index ... ok
test rank::tests::test_nullable_booleans ... ok
test rank::tests::test_primitive ... ok
```
--
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] make_decoder accepts borrowed DataType instead of owned [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #9270:
URL: https://github.com/apache/arrow-rs/pull/9270#issuecomment-3802065609
@alamb should be good now
--
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] make_decoder accepts borrowed DataType instead of owned [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9270:
URL: https://github.com/apache/arrow-rs/pull/9270#discussion_r2729022461
##########
arrow-json/src/reader/mod.rs:
##########
@@ -295,16 +296,15 @@ impl ReaderBuilder {
/// Create a [`Decoder`]
pub fn build_decoder(self) -> Result<Decoder, ArrowError> {
- let (data_type, nullable) = match self.is_field {
- false => (DataType::Struct(self.schema.fields.clone()), false),
- true => {
- let field = &self.schema.fields[0];
- (field.data_type().clone(), field.is_nullable())
- }
+ let (data_type, nullable) = if self.is_field {
+ let field = &self.schema.fields[0];
+ (Cow::Borrowed(field.data_type()), field.is_nullable())
Review Comment:
NOTE: This `Cow::Borrowed` is necessary to preserve pointer stability of `field.data_type()` that would otherwise have to be cloned.
--
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] make_decoder accepts borrowed DataType instead of owned [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9270:
URL: https://github.com/apache/arrow-rs/pull/9270#discussion_r2729516637
##########
arrow-json/src/reader/struct_array.rs:
##########
@@ -81,28 +81,30 @@ pub struct StructArrayDecoder {
impl StructArrayDecoder {
pub fn new(
ctx: &DecoderContext,
- data_type: DataType,
+ data_type: &DataType,
is_nullable: bool,
) -> Result<Self, ArrowError> {
- let struct_mode = ctx.struct_mode();
- let fields = struct_fields(&data_type);
-
- // If this struct nullable, need to permit nullability in child array
- // StructArrayDecoder::decode verifies that if the child is not nullable
- // it doesn't contain any nulls not masked by its parent
Review Comment:
https://github.com/apache/arrow-rs/pull/9271 merged too quickly... this comment was supposed to remain inside the `.map` call. So I'm restoring this code back to follow the original upstream approach, instead of opening a separate PR just for that.
--
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] make_decoder accepts borrowed DataType instead of owned [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9270:
URL: https://github.com/apache/arrow-rs/pull/9270#discussion_r2729516637
##########
arrow-json/src/reader/struct_array.rs:
##########
@@ -81,28 +81,30 @@ pub struct StructArrayDecoder {
impl StructArrayDecoder {
pub fn new(
ctx: &DecoderContext,
- data_type: DataType,
+ data_type: &DataType,
is_nullable: bool,
) -> Result<Self, ArrowError> {
- let struct_mode = ctx.struct_mode();
- let fields = struct_fields(&data_type);
-
- // If this struct nullable, need to permit nullability in child array
- // StructArrayDecoder::decode verifies that if the child is not nullable
- // it doesn't contain any nulls not masked by its parent
Review Comment:
https://github.com/apache/arrow-rs/pull/9271 merged too quickly... this comment was supposed to remain inside the `.map` call. So I'm restoring it here instead of opening a separate PR just for that.
--
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] make_decoder accepts borrowed DataType instead of owned [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9270:
URL: https://github.com/apache/arrow-rs/pull/9270#issuecomment-3802576870
> @alamb -- Is it normal for MIRI to take O(hours)? It's been stuck here for a very long time:
It does take a while. I haven't tracked its overall time recently -- maybe some new tests are overly long
--
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