You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jecsand838 (via GitHub)" <gi...@apache.org> on 2026/01/21 10:59:57 UTC
[PR] Add tests and fixes for schema resolution bug [arrow-rs]
jecsand838 opened a new pull request, #9237:
URL: https://github.com/apache/arrow-rs/pull/9237
# Which issue does this PR close?
- Closes #9231.
# Rationale for this change
Avro schema resolution allows a reader schema to represent “nullable” values using a two-branch union (`["null", T]` or `[T, "null"]`) while still reading data written with the non-union schema `T` (i.e. without union discriminants in the encoded data).
In `arrow-avro`, resolving a non-union writer type against a reader union (notably for array/list item schemas like `items: ["null", "int"]`) could incorrectly treat the encoded stream as a union and attempt to decode a union discriminant. This would misalign decoding and could surface as `ParseError("bad varint")` for certain files (see #9231).
# What changes are included in this PR?
- Fix schema resolution when the *writer* schema is non-union and the *reader* schema is a union:
- Special-case two-branch unions containing `null` to be treated as “nullable” (capturing whether `null` is first or second), and resolve against the non-null branch.
- Improve matching for general reader unions by attempting to resolve against each union variant, preferring a direct match, and constructing the appropriate union resolution mapping for the selected branch.
- Ensure promotions are represented at the union-resolution level (avoiding nested promotion resolution on the selected union child).
- Add regression coverage for the bug and the fixed behavior:
- `test_resolve_array_writer_nonunion_items_reader_nullable_items` (schema resolution / codec)
- `test_array_decoding_writer_nonunion_items_reader_nullable_items` (record decoding; ensures correct byte consumption and decoded values)
- `test_bad_varint_bug_nullable_array_items` (end-to-end reader regression using a small Avro fixture)
- Add a small compressed Avro fixture under `arrow-avro/test/data/bad-varint-bug.avro.gz` used by the regression test.
# Are these changes tested?
Yes. This PR adds targeted unit/integration tests that reproduce the prior failure mode and validate correct schema resolution and decoding for nullable-union array items.
# Are there any user-facing changes?
Yes (bug fix): reading Avro files with arrays whose element type is represented as a nullable union in the reader schema (e.g. `items: ["null", "int"]`) now succeeds instead of failing with `ParseError("bad varint")`. No public API changes are intended.
--
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] Add tests and fixes for schema resolution bug [arrow-rs]
Posted by "mzabaluev (via GitHub)" <gi...@apache.org>.
mzabaluev commented on code in PR #9237:
URL: https://github.com/apache/arrow-rs/pull/9237#discussion_r2712165072
##########
arrow-avro/src/reader/mod.rs:
##########
@@ -9081,4 +9081,46 @@ mod test {
"entire RecordBatch mismatch (schema, all columns, all rows)"
);
}
+
+ #[test]
+ fn test_bad_varint_bug_nullable_array_items() {
+ use flate2::read::GzDecoder;
+ use std::io::Read;
+ let manifest_dir = env!("CARGO_MANIFEST_DIR");
+ let gz_path = format!("{manifest_dir}/test/data/bad-varint-bug.avro.gz");
+ let gz_file = File::open(&gz_path).expect("test file should exist");
+ let mut decoder = GzDecoder::new(gz_file);
Review Comment:
I actually had the file unpacked in my test suite branch, it's quite small. I only gzipped it to be able to post to GitHub. But if there's already a dependency on `flate2`, why not use it?
##########
arrow-avro/src/codec.rs:
##########
@@ -1529,23 +1529,77 @@ impl<'a> Maker<'a> {
Ok(dt)
}
(writer_non_union, Schema::Union(reader_variants)) => {
- let promo = self.find_best_promotion(
- writer_non_union,
- reader_variants.as_slice(),
- namespace,
- );
- let Some((reader_index, promotion)) = promo else {
- return Err(ArrowError::SchemaError(
- "Writer schema does not match any reader union branch".to_string(),
- ));
- };
- let mut dt = self.parse_type(reader_schema, namespace)?;
- dt.resolution = Some(ResolutionInfo::Union(ResolvedUnion {
- writer_to_reader: Arc::from(vec![Some((reader_index, promotion))]),
- writer_is_union: false,
- reader_is_union: true,
- }));
- Ok(dt)
+ let null_position = reader_variants
+ .iter()
+ .position(|x| x == &Schema::TypeName(TypeName::Primitive(PrimitiveType::Null)));
+ if let (2, Some(null_idx)) = (reader_variants.len(), null_position) {
Review Comment:
Would it be good to reuse the `nullable_union_variants` helper here?
##########
arrow-avro/src/codec.rs:
##########
@@ -1529,23 +1529,77 @@ impl<'a> Maker<'a> {
Ok(dt)
}
(writer_non_union, Schema::Union(reader_variants)) => {
- let promo = self.find_best_promotion(
- writer_non_union,
- reader_variants.as_slice(),
- namespace,
- );
- let Some((reader_index, promotion)) = promo else {
- return Err(ArrowError::SchemaError(
- "Writer schema does not match any reader union branch".to_string(),
- ));
- };
- let mut dt = self.parse_type(reader_schema, namespace)?;
- dt.resolution = Some(ResolutionInfo::Union(ResolvedUnion {
- writer_to_reader: Arc::from(vec![Some((reader_index, promotion))]),
- writer_is_union: false,
- reader_is_union: true,
- }));
- Ok(dt)
+ let null_position = reader_variants
+ .iter()
+ .position(|x| x == &Schema::TypeName(TypeName::Primitive(PrimitiveType::Null)));
+ if let (2, Some(null_idx)) = (reader_variants.len(), null_position) {
+ let non_null_idx = 1 - null_idx;
+ let non_null_branch = &reader_variants[non_null_idx];
+ let mut dt =
+ self.make_data_type(writer_non_union, Some(non_null_branch), namespace)?;
Review Comment:
I call `resolve_type` directly here because that's what it always amounts to.
--
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] Add tests and fixes for schema resolution bug [arrow-rs]
Posted by "mzabaluev-flarion (via GitHub)" <gi...@apache.org>.
mzabaluev-flarion commented on code in PR #9237:
URL: https://github.com/apache/arrow-rs/pull/9237#discussion_r2712893496
##########
arrow-avro/src/codec.rs:
##########
@@ -1529,23 +1529,77 @@ impl<'a> Maker<'a> {
Ok(dt)
}
(writer_non_union, Schema::Union(reader_variants)) => {
- let promo = self.find_best_promotion(
- writer_non_union,
- reader_variants.as_slice(),
- namespace,
- );
- let Some((reader_index, promotion)) = promo else {
- return Err(ArrowError::SchemaError(
- "Writer schema does not match any reader union branch".to_string(),
- ));
- };
- let mut dt = self.parse_type(reader_schema, namespace)?;
- dt.resolution = Some(ResolutionInfo::Union(ResolvedUnion {
- writer_to_reader: Arc::from(vec![Some((reader_index, promotion))]),
- writer_is_union: false,
- reader_is_union: true,
- }));
- Ok(dt)
+ let null_position = reader_variants
+ .iter()
+ .position(|x| x == &Schema::TypeName(TypeName::Primitive(PrimitiveType::Null)));
+ if let (2, Some(null_idx)) = (reader_variants.len(), null_position) {
+ let non_null_idx = 1 - null_idx;
+ let non_null_branch = &reader_variants[non_null_idx];
+ let mut dt =
+ self.make_data_type(writer_non_union, Some(non_null_branch), namespace)?;
+ dt.nullability = Some(if null_idx == 0 {
+ Nullability::NullFirst
+ } else {
+ Nullability::NullSecond
+ });
+ #[cfg(feature = "avro_custom_types")]
+ if let Some(nb) = dt.nullability {
+ Self::propagate_nullability_into_ree(&mut dt, nb);
+ }
Review Comment:
```suggestion
let nullability = if null_idx == 0 {
Nullability::NullFirst
} else {
Nullability::NullSecond
};
#[cfg(feature = "avro_custom_types")]
Self::propagate_nullability_into_ree(&mut dt, nullability);
dt.nullability = Some(nullability);
```
--
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] Add tests and fixes for schema resolution bug [arrow-rs]
Posted by "mzabaluev (via GitHub)" <gi...@apache.org>.
mzabaluev commented on code in PR #9237:
URL: https://github.com/apache/arrow-rs/pull/9237#discussion_r2712165072
##########
arrow-avro/src/reader/mod.rs:
##########
@@ -9081,4 +9081,46 @@ mod test {
"entire RecordBatch mismatch (schema, all columns, all rows)"
);
}
+
+ #[test]
+ fn test_bad_varint_bug_nullable_array_items() {
+ use flate2::read::GzDecoder;
+ use std::io::Read;
+ let manifest_dir = env!("CARGO_MANIFEST_DIR");
+ let gz_path = format!("{manifest_dir}/test/data/bad-varint-bug.avro.gz");
+ let gz_file = File::open(&gz_path).expect("test file should exist");
+ let mut decoder = GzDecoder::new(gz_file);
Review Comment:
I actually had the file unpacked in my test suite branch, it's quite small. I only gzipped it to be able to post to GitHub. But since there's already a dependency on `flate2`, might as well use it.
--
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] Add tests and fixes for schema resolution bug [arrow-rs]
Posted by "mzabaluev (via GitHub)" <gi...@apache.org>.
mzabaluev commented on code in PR #9237:
URL: https://github.com/apache/arrow-rs/pull/9237#discussion_r2712169035
##########
arrow-avro/src/codec.rs:
##########
@@ -1529,23 +1529,77 @@ impl<'a> Maker<'a> {
Ok(dt)
}
(writer_non_union, Schema::Union(reader_variants)) => {
- let promo = self.find_best_promotion(
- writer_non_union,
- reader_variants.as_slice(),
- namespace,
- );
- let Some((reader_index, promotion)) = promo else {
- return Err(ArrowError::SchemaError(
- "Writer schema does not match any reader union branch".to_string(),
- ));
- };
- let mut dt = self.parse_type(reader_schema, namespace)?;
- dt.resolution = Some(ResolutionInfo::Union(ResolvedUnion {
- writer_to_reader: Arc::from(vec![Some((reader_index, promotion))]),
- writer_is_union: false,
- reader_is_union: true,
- }));
- Ok(dt)
+ let null_position = reader_variants
+ .iter()
+ .position(|x| x == &Schema::TypeName(TypeName::Primitive(PrimitiveType::Null)));
+ if let (2, Some(null_idx)) = (reader_variants.len(), null_position) {
+ let non_null_idx = 1 - null_idx;
+ let non_null_branch = &reader_variants[non_null_idx];
+ let mut dt =
+ self.make_data_type(writer_non_union, Some(non_null_branch), namespace)?;
Review Comment:
I'd call `resolve_type` directly here because that's what it always amounts to.
--
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] Add tests and fixes for schema resolution bug [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9237:
URL: https://github.com/apache/arrow-rs/pull/9237#discussion_r2725584701
##########
arrow-avro/test/data/bad-varint-bug.avro.gz:
##########
Review Comment:
I unzipped this file and it looks fine to me
```
00000000 4f 62 6a 01 06 16 61 76 72 6f 2e 73 63 68 65 6d |Obj...avro.schem|
00000010 61 84 02 7b 22 74 79 70 65 22 3a 22 72 65 63 6f |a..{"type":"reco|
00000020 72 64 22 2c 22 6e 61 6d 65 22 3a 22 74 61 62 6c |rd","name":"tabl|
00000030 65 22 2c 22 66 69 65 6c 64 73 22 3a 5b 7b 22 6e |e","fields":[{"n|
00000040 61 6d 65 22 3a 22 69 6e 74 5f 61 72 72 61 79 22 |ame":"int_array"|
00000050 2c 22 74 79 70 65 22 3a 7b 22 74 79 70 65 22 3a |,"type":{"type":|
00000060 22 61 72 72 61 79 22 2c 22 69 74 65 6d 73 22 3a |"array","items":|
00000070 22 69 6e 74 22 2c 22 65 6c 65 6d 65 6e 74 2d 69 |"int","element-i|
00000080 64 22 3a 32 7d 2c 22 66 69 65 6c 64 2d 69 64 22 |d":2},"field-id"|
00000090 3a 31 7d 5d 7d 14 61 76 72 6f 2e 63 6f 64 65 63 |:1}]}.avro.codec|
000000a0 0e 64 65 66 6c 61 74 65 1c 69 63 65 62 65 72 67 |.deflate.iceberg|
000000b0 2e 73 63 68 65 6d 61 c8 02 7b 22 74 79 70 65 22 |.schema..{"type"|
000000c0 3a 22 73 74 72 75 63 74 22 2c 22 73 63 68 65 6d |:"struct","schem|
000000d0 61 2d 69 64 22 3a 30 2c 22 66 69 65 6c 64 73 22 |a-id":0,"fields"|
000000e0 3a 5b 7b 22 69 64 22 3a 31 2c 22 6e 61 6d 65 22 |:[{"id":1,"name"|
000000f0 3a 22 69 6e 74 5f 61 72 72 61 79 22 2c 22 72 65 |:"int_array","re|
00000100 71 75 69 72 65 64 22 3a 74 72 75 65 2c 22 74 79 |quired":true,"ty|
00000110 70 65 22 3a 7b 22 74 79 70 65 22 3a 22 6c 69 73 |pe":{"type":"lis|
00000120 74 22 2c 22 65 6c 65 6d 65 6e 74 2d 69 64 22 3a |t","element-id":|
00000130 32 2c 22 65 6c 65 6d 65 6e 74 22 3a 22 69 6e 74 |2,"element":"int|
00000140 22 2c 22 65 6c 65 6d 65 6e 74 2d 72 65 71 75 69 |","element-requi|
00000150 72 65 64 22 3a 74 72 75 65 7d 7d 5d 7d 00 24 70 |red":true}}]}.$p|
00000160 9c 93 05 f1 5e 13 2f c5 a0 60 cd 7a a0 35 02 0c |....^./..`.z.5..|
00000170 63 61 62 61 00 00 24 70 9c 93 05 f1 5e 13 2f c5 |caba..$p....^./.|
00000180 a0 60 cd 7a a0 35 |.`.z.5|
```
--
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] Add tests and fixes for schema resolution bug [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9237:
URL: https://github.com/apache/arrow-rs/pull/9237#issuecomment-3801925689
Thanks @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] Add tests and fixes for schema resolution bug [arrow-rs]
Posted by "jecsand838 (via GitHub)" <gi...@apache.org>.
jecsand838 commented on PR #9237:
URL: https://github.com/apache/arrow-rs/pull/9237#issuecomment-3795710123
@alamb @mbrobbel
Would it be possible to get this bug fix in for the 58.0.0 release 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] Add tests and fixes for schema resolution bug [arrow-rs]
Posted by "jecsand838 (via GitHub)" <gi...@apache.org>.
jecsand838 commented on PR #9237:
URL: https://github.com/apache/arrow-rs/pull/9237#issuecomment-3780745771
@mzabaluev Thank you for the review! I just pushed up a commit that addresses your comments. Let me know what you think when you get a chance.
--
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] Add tests and fixes for schema resolution bug [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #9237:
URL: https://github.com/apache/arrow-rs/pull/9237
--
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