You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "ariel-miculas (via GitHub)" <gi...@apache.org> on 2026/03/23 15:28:06 UTC
[PR] fix: use writer types in Skipper for resolved named record types [arrow-rs]
ariel-miculas opened a new pull request, #9605:
URL: https://github.com/apache/arrow-rs/pull/9605
# Which issue does this PR close?
# Rationale for this change
When a writer-only field references a named Avro type that was previously resolved against a reader schema, `parse_type` returns the registered reader-resolved type from the shared resolver. This caused two problems:
1. The Skipper built its struct sub-skippers from the reader's field list, which omits writer-only fields. Their bytes were never consumed, leaving the cursor at the wrong position for all subsequent records.
2. Reader fields carry resolution-induced nullability (e.g. a writer plain `long` matched against a reader `["null", long]` gains `nullability = Some(NullFirst)`). The Skipper read a union-tag byte that was never written, causing "Unexpected EOF" errors.
Fix: store the writer's data type in `ResolvedField::ToReader` alongside the reader index. The Skipper's `Codec::Struct` arm now iterates `rec.writer_fields` and uses the writer type from every entry - both `ToReader(_, wdt)` and `Skip(wdt)` - so it always follows the writer's wire format.
# What changes are included in this PR?
# Are these changes tested?
Yes, added unit tests.
# Are there any user-facing changes?
No
--
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] fix: use writer types in Skipper for resolved named record types [arrow-rs]
Posted by "ariel-miculas (via GitHub)" <gi...@apache.org>.
ariel-miculas commented on PR #9605:
URL: https://github.com/apache/arrow-rs/pull/9605#issuecomment-4192581277
@alamb please let me know if the new tests are good
--
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] fix: use writer types in Skipper for resolved named record types [arrow-rs]
Posted by "ariel-miculas (via GitHub)" <gi...@apache.org>.
ariel-miculas commented on PR #9605:
URL: https://github.com/apache/arrow-rs/pull/9605#issuecomment-4177518905
These tests fail on main:
```
$ cargo test -p arrow-avro --lib test_nullable_reader_schema_vs_plain_writer_nested_struct
Compiling arrow-avro v58.1.0 (/Users/amiculas/work/arrow-rs/arrow-avro)
Finished `test` profile [unoptimized + debuginfo] target(s) in 2.51s
Running unittests src/lib.rs (target/debug/deps/arrow_avro-3c0d1f09f75a0c0a)
running 1 test
test reader::test::test_nullable_reader_schema_vs_plain_writer_nested_struct ... FAILED
failures:
---- reader::test::test_nullable_reader_schema_vs_plain_writer_nested_struct stdout ----
thread 'reader::test::test_nullable_reader_schema_vs_plain_writer_nested_struct' (11467926) panicked at arrow-avro/src/reader/mod.rs:9670:14:
reading should succeed: AvroError("Parser error: bad varint")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
reader::test::test_nullable_reader_schema_vs_plain_writer_nested_struct
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 392 filtered out; finished in 0.01s
error: test failed, to rerun pass `-p arrow-avro --lib`
~/work/arrow-rs main* ⇡
$ cargo test -p arrow-avro --lib test_skip_array_of_structs_uses_writer_schema_not_resolved
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.09s
Running unittests src/lib.rs (target/debug/deps/arrow_avro-3c0d1f09f75a0c0a)
running 1 test
test reader::test::test_skip_array_of_structs_uses_writer_schema_not_resolved ... FAILED
failures:
---- reader::test::test_skip_array_of_structs_uses_writer_schema_not_resolved stdout ----
thread 'reader::test::test_skip_array_of_structs_uses_writer_schema_not_resolved' (11468665) panicked at arrow-avro/src/reader/mod.rs:9793:14:
Skipper must consume all events bytes using writer field types: AvroError("Parser error: bad varint")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
reader::test::test_skip_array_of_structs_uses_writer_schema_not_resolved
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 392 filtered out; finished in 0.00s
error: test failed, to rerun pass `-p arrow-avro --lib`
~/work/arrow-rs main* ⇡
$ cargo test -p arrow-avro --lib test_skipper_consumes_writer_only_struct_fields
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.08s
Running unittests src/lib.rs (target/debug/deps/arrow_avro-3c0d1f09f75a0c0a)
running 1 test
test reader::test::test_skipper_consumes_writer_only_struct_fields has been running for over 60 seconds
^C
```
with the last one stuck in an infinite loop
--
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] fix: use writer types in Skipper for resolved named record types [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9605:
URL: https://github.com/apache/arrow-rs/pull/9605#issuecomment-4138663816
FYI @jecsand838
--
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] fix: use writer types in Skipper for resolved named record types [arrow-rs]
Posted by "jecsand838 (via GitHub)" <gi...@apache.org>.
jecsand838 commented on PR #9605:
URL: https://github.com/apache/arrow-rs/pull/9605#issuecomment-4172638199
@ariel-miculas
I think the correctness fix here is right.
One improvement I'd recommend is pushing the writer-wire planning fully into `codec.rs`, instead of carrying full `AvroDataType`s down into `record.rs` and having `Skipper::from_avro` reconstruct the skip tree there.
That would keep the projection layer working with a precomputed writer-side plan and should shave some decoder-construction work.
Maybe something like this in `codec.rs`:
```rust
#[derive(Debug, Clone, PartialEq)]
pub(crate) enum WriterSkipPlan {
Null,
Boolean,
Int32,
Int64,
Float32,
Float64,
Bytes,
String,
TimeMicros,
TimestampMillis,
TimestampMicros,
TimestampNanos,
Fixed(usize),
Decimal(Option<usize>),
UuidString,
Enum,
List(Box<WriterSkipPlan>),
Map(Box<WriterSkipPlan>),
Struct(Arc<[WriterSkipPlan]>),
Union(Arc<[WriterSkipPlan]>),
Nullable(Nullability, Box<WriterSkipPlan>),
DurationFixed12,
#[cfg(feature = "avro_custom_types")]
RunEndEncoded(Box<WriterSkipPlan>),
}
#[derive(Debug, Clone, PartialEq)]
pub(crate) enum ResolvedField {
ToReader(usize, WriterSkipPlan),
Skip(WriterSkipPlan),
}
````
and then materialize the writer-side skip tree in `codec.rs` once:
```rust
impl AvroDataType {
fn writer_skip_plan(&self) -> Result<WriterSkipPlan, ArrowError> {
let mut plan = match self.codec() {
Codec::Null => WriterSkipPlan::Null,
Codec::Boolean => WriterSkipPlan::Boolean,
Codec::Int32 | Codec::Date32 | Codec::TimeMillis => WriterSkipPlan::Int32,
Codec::Int64 => WriterSkipPlan::Int64,
Codec::TimeMicros => WriterSkipPlan::TimeMicros,
Codec::TimestampMillis(_) => WriterSkipPlan::TimestampMillis,
Codec::TimestampMicros(_) => WriterSkipPlan::TimestampMicros,
Codec::TimestampNanos(_) => WriterSkipPlan::TimestampNanos,
#[cfg(feature = "avro_custom_types")]
Codec::DurationNanos
| Codec::DurationMicros
| Codec::DurationMillis
| Codec::DurationSeconds => WriterSkipPlan::Int64,
#[cfg(feature = "avro_custom_types")]
Codec::Int8 | Codec::Int16 | Codec::UInt8 | Codec::UInt16 | Codec::Time32Secs => {
WriterSkipPlan::Int32
}
#[cfg(feature = "avro_custom_types")]
Codec::UInt32 | Codec::Date64 | Codec::TimeNanos | Codec::TimestampSecs(_) => {
WriterSkipPlan::Int64
}
#[cfg(feature = "avro_custom_types")]
Codec::UInt64 => WriterSkipPlan::Fixed(8),
#[cfg(feature = "avro_custom_types")]
Codec::Float16 => WriterSkipPlan::Fixed(2),
#[cfg(feature = "avro_custom_types")]
Codec::IntervalYearMonth => WriterSkipPlan::Fixed(4),
#[cfg(feature = "avro_custom_types")]
Codec::IntervalMonthDayNano => WriterSkipPlan::Fixed(16),
#[cfg(feature = "avro_custom_types")]
Codec::IntervalDayTime => WriterSkipPlan::Fixed(8),
Codec::Float32 => WriterSkipPlan::Float32,
Codec::Float64 => WriterSkipPlan::Float64,
Codec::Binary => WriterSkipPlan::Bytes,
Codec::Utf8 | Codec::Utf8View => WriterSkipPlan::String,
Codec::Fixed(sz) => WriterSkipPlan::Fixed(*sz as usize),
Codec::Decimal(_, _, size) => WriterSkipPlan::Decimal(*size),
Codec::Uuid => WriterSkipPlan::UuidString,
Codec::Enum(_) => WriterSkipPlan::Enum,
Codec::List(item) => WriterSkipPlan::List(Box::new(item.writer_skip_plan()?)),
Codec::Map(values) => WriterSkipPlan::Map(Box::new(values.writer_skip_plan()?)),
Codec::Struct(fields) => {
let children = if let Some(ResolutionInfo::Record(rec)) = self.resolution.as_ref() {
rec.writer_fields
.iter()
.map(|f| match f {
ResolvedField::ToReader { writer_plan, .. }
| ResolvedField::Skip(writer_plan) => Ok(writer_plan.clone()),
})
.collect::<Result<Arc<[_]>, ArrowError>>()?
} else {
fields
.iter()
.map(|f| f.data_type().writer_skip_plan())
.collect::<Result<Arc<[_]>, ArrowError>>()?
};
WriterSkipPlan::Struct(children)
}
Codec::Interval => WriterSkipPlan::DurationFixed12,
Codec::Union(encodings, _, _) => {
let children = encodings
.iter()
.map(|e| e.writer_skip_plan())
.collect::<Result<Arc<[_]>, ArrowError>>()?;
WriterSkipPlan::Union(children)
}
#[cfg(feature = "avro_custom_types")]
Codec::RunEndEncoded(values, _) => {
WriterSkipPlan::RunEndEncoded(Box::new(values.writer_skip_plan()?))
}
};
if let Some(nullability) = self.nullability() {
plan = WriterSkipPlan::Nullable(nullability, Box::new(plan));
}
Ok(plan)
}
}
```
`resolve_records` can also stash the precomputed writer plan instead of a full `AvroDataType`:
```rust
let writer_fields = writer_record
.fields
.iter()
.enumerate()
.map(|(writer_index, writer_field)| {
let dt = self.parse_type(&writer_field.r#type, writer_ns)?;
let writer_plan = dt.writer_skip_plan()?;
Ok(match writer_to_reader[writer_index] {
Some(reader_index) => ResolvedField::ToReader(
reader_index,
writer_plan,
),
None => ResolvedField::Skip(writer_plan),
})
})
.collect::<Result<_, ArrowError>>()?;
```
That way `record.rs` can consume a prebuilt writer-side plan instead of reconstructing it from `AvroDataType` + `ResolutionInfo::Record`.
We can also implement these improvements in a follow-up as well.
--
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] fix: use writer types in Skipper for resolved named record types [arrow-rs]
Posted by "ariel-miculas (via GitHub)" <gi...@apache.org>.
ariel-miculas commented on PR #9605:
URL: https://github.com/apache/arrow-rs/pull/9605#issuecomment-4161803235
fixed formatting and rebased onto main;
@jecsand838 can you please take a look?
--
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] fix: use writer types in Skipper for resolved named record types [arrow-rs]
Posted by "ariel-miculas (via GitHub)" <gi...@apache.org>.
ariel-miculas commented on code in PR #9605:
URL: https://github.com/apache/arrow-rs/pull/9605#discussion_r3027782964
##########
arrow-avro/src/reader/record.rs:
##########
@@ -5611,4 +5629,788 @@ mod tests {
other => panic!("expected Timestamp(Nanosecond, None), got {other:?}"),
}
}
+
+ /// When a Skip field references a named type by name, the Skipper must use the
+ /// writer's wire format for that type. Here the reader wraps the Timestamp's scalar
+ /// fields in nullable unions, but the writer wrote them as plain values; the Skipper
+ /// must not add a union-tag read for each field.
+ #[test]
Review Comment:
rewrote the tests from a higher level POV and opened https://github.com/apache/arrow-rs/issues/9655
--
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] fix: use writer types in Skipper for resolved named record types [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #9605:
URL: https://github.com/apache/arrow-rs/pull/9605
--
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] fix: use writer types in Skipper for resolved named record types [arrow-rs]
Posted by "jecsand838 (via GitHub)" <gi...@apache.org>.
jecsand838 commented on PR #9605:
URL: https://github.com/apache/arrow-rs/pull/9605#issuecomment-4173278884
> Thank you for this PR @ariel-miculas 🙏
>
> I defer to @jecsand838 's opinion on code structure
>
> Before approving this PR I would would request that we:
>
> 1. Try and make the tests easier to understand (see comemnts below)
> 2. File a ticket that explains what the end user of this crate would see before this code fix (I can't quite tell from the PR description which focuses on what the code does, not the end user visible behavior)
>
> It would also be nice to file a ticket to track @jecsand838 's suggestion for a different structure
@alamb I created that tracking ticket: https://github.com/apache/arrow-rs/issues/9651
--
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] fix: use writer types in Skipper for resolved named record types [arrow-rs]
Posted by "ariel-miculas (via GitHub)" <gi...@apache.org>.
ariel-miculas commented on PR #9605:
URL: https://github.com/apache/arrow-rs/pull/9605#issuecomment-4172967750
> However this can always be done in a follow-up as well.
I'd prefer it this way, since refactoring would take a bit more consideration.
--
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] fix: use writer types in Skipper for resolved named record types [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9605:
URL: https://github.com/apache/arrow-rs/pull/9605#discussion_r3024784220
##########
arrow-avro/src/reader/record.rs:
##########
@@ -5611,4 +5629,788 @@ mod tests {
other => panic!("expected Timestamp(Nanosecond, None), got {other:?}"),
}
}
+
+ /// When a Skip field references a named type by name, the Skipper must use the
+ /// writer's wire format for that type. Here the reader wraps the Timestamp's scalar
+ /// fields in nullable unions, but the writer wrote them as plain values; the Skipper
+ /// must not add a union-tag read for each field.
+ #[test]
Review Comment:
these tests seem pretty repetitive and thus it is hard for me to understand what they are supposed to be testing and validate what is different between tehm
Can you please refactor the common functionality into helper functions so the tests themselves are easier to understand and validate ?
--
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