mbutrovich commented on code in PR #3354:
URL: https://github.com/apache/iceberg-rust/pull/3354#discussion_r4224926808
##########
crates/iceberg/src/spec/manifest/mod.rs:
##########
@@ -45,9 +52,45 @@ 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) {
Review Comment:
> I'd move the swap/log into the success path after the retry parse returns
Ok.
[Fixed](https://github.com/apache/iceberg-rust/blob/95667f5be29b8c24a8a5155b80c1fb5986a1c0d0/crates/iceberg/src/spec/manifest/mod.rs#L67-L99).
The fallback logs and sets `WARNED_REPEATED_DEFINITIONS` only after the retry
`AvroReader::new` succeeds.
[`test_parse_manifest_with_repeated_named_type_definitions_reports_original_error`](https://github.com/apache/iceberg-rust/blob/95667f5be29b8c24a8a5155b80c1fb5986a1c0d0/crates/iceberg/src/spec/manifest/mod.rs#L2245-L2276)
now captures the log at debug level and checks that a failed retry logs
nothing about the repeated definitions.
--
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]
