mbutrovich commented on code in PR #3354:
URL: https://github.com/apache/iceberg-rust/pull/3354#discussion_r4209481976
##########
crates/iceberg/src/spec/manifest/mod.rs:
##########
@@ -45,9 +52,40 @@ pub struct Manifest {
}
impl Manifest {
- /// Parse manifest metadata and entries from bytes of avro file.
- pub(crate) fn try_from_avro_bytes(bs: &[u8]) -> Result<(ManifestMetadata,
Vec<ManifestEntry>)> {
- let reader = AvroReader::new(bs)?;
+ /// Parse manifest metadata and entries from bytes of avro file. `location`
+ /// names the manifest in warnings.
+ pub(crate) fn try_from_avro_bytes(
+ bs: &[u8],
+ location: Option<&str>,
+ ) -> Result<(ManifestMetadata, Vec<ManifestEntry>)> {
+ let rewritten;
+ let reader = match AvroReader::new(bs) {
+ Ok(reader) => reader,
+ // iceberg-rust repeated `decimal` definitions before
+ // `schema_to_avro_schema` defined each named type once, so this
+ // fallback stays while tables can contain manifests it wrote.
+ Err(e) if matches!(e.details(),
Details::AmbiguousSchemaDefinition(_)) => {
+ let Ok(Some((bs, repeated))) = define_named_types_once(bs)
else {
+ return Err(e.into());
+ };
+ let location = location.unwrap_or("<unknown location>");
+ if WARNED_REPEATED_DEFINITIONS.swap(true, Ordering::Relaxed) {
+ tracing::debug!(
+ "Manifest {location} defines Avro named types
{repeated:?} more than once."
+ );
+ } else {
+ tracing::warn!(
+ "Manifest {location} defines Avro named types
{repeated:?} more than once, \
+ which the Avro specification doesn't allow. Reading
it with each repeated \
+ definition replaced by a reference to the first.
Later manifests like \
+ this are logged at debug level."
+ );
+ }
+ rewritten = bs;
+ AvroReader::new(rewritten.as_slice())?
Review Comment:
> if the rewrite succeeds but this reparse fails, `?` hands back the
secondary error, not the `AmbiguousSchemaDefinition` that sent us into the
fallback.
[Fixed](https://github.com/apache/iceberg-rust/blob/fefcbd6d17a262ce240a6b3ae0ee99164d8f6fed/crates/iceberg/src/spec/manifest/mod.rs#L85-L90).
It returns the `AmbiguousSchemaDefinition` error and adds the reparse error as
context. The reparse can fail only on a second defect in the file, such as a
truncated sync marker, so I kept that error too.
[`test_parse_manifest_with_repeated_named_type_definitions_reports_original_error`](https://github.com/apache/iceberg-rust/blob/fefcbd6d17a262ce240a6b3ae0ee99164d8f6fed/crates/iceberg/src/spec/manifest/mod.rs#L2235-L2257)
cuts the header's sync marker in half and checks that the error includes both.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
