mbutrovich commented on code in PR #3241:
URL: https://github.com/apache/iceberg-rust/pull/3241#discussion_r4126287175
##########
bindings/python/src/manifest.rs:
##########
@@ -195,11 +195,10 @@ impl PyManifestEntry {
}
#[pyfunction]
-pub fn read_manifest_entries(bs: &[u8]) -> PyManifest {
- // TODO: Some error handling
- PyManifest {
- inner: Manifest::parse_avro(bs).unwrap(),
- }
+pub fn read_manifest_entries(bs: &[u8]) -> PyResult<PyManifest> {
+ Ok(PyManifest {
+ inner: Manifest::parse_avro(bs).map_err(to_py_err)?,
+ })
}
Review Comment:
Should the description say `Part of #3240` instead of `Closes #3240`? As I
read #3240, the goal is for bad input to raise an error instead of panicking.
After this PR the binding itself doesn't panic, but `apache-avro` 0.21, the
version on `main`
([`Cargo.toml#L49`](https://github.com/apache/iceberg-rust/blob/86d6618804a902eac10f09c09a565aca5dad0d46/Cargo.toml#L49)),
still panics on some malformed files.
I truncated and bit-flipped the two V2 manifest lists in
`crates/iceberg/testdata/manifests_lists` and a V2 manifest from the
`spec::manifest` tests, which comes to about 51,000 inputs. Twelve panicked,
and all twelve panicked in `Name::parse`, which calls `unwrap()` when a record
name in the file's Avro schema fails validation
([`schema.rs#L266-L273`](https://github.com/apache/avro-rs/blob/04707999f75278fdea927ee8d2a59de41d8f22a7/avro/src/schema.rs#L266-L273)).
One example is a record name that becomes `manifest^file`. In 0.22,
`Name::parse` returns that error instead
([`name.rs#L112-L121`](https://github.com/apache/avro-rs/blob/ec5721cb0c80dcde56c1049a004f1d785abd88cf/avro/src/schema/name.rs#L112-L121)),
so the upgrade in #3063 should fix it. Keeping #3240 open until then would
keep that panic tracked.
--
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]
