mbutrovich commented on code in PR #3241:
URL: https://github.com/apache/iceberg-rust/pull/3241#discussion_r4126278232
##########
bindings/python/src/data_file.rs:
##########
@@ -120,20 +122,20 @@ impl PyDataFile {
}
#[getter]
- fn upper_bounds(&self) -> HashMap<i32, Vec<u8>> {
+ fn upper_bounds(&self) -> PyResult<HashMap<i32, Vec<u8>>> {
Review Comment:
> i noticed theres no coverage for `upper_bounds`/`lower_bounds`, might be a
good follow up
`test_read_manifest_entry` covers the success path for both getters
([`test_manifest.py#L165-L172`](https://github.com/apache/iceberg-rust/blob/104d95911b832b107a7a5b747de184fd08757ea8/bindings/python/tests/test_manifest.py#L165-L172)).
Could we also cover the new error path in this PR? A parsed manifest can reach
it. When iceberg-rust reads the table schema that a manifest stores in its Avro
file metadata, it doesn't check decimal precision
([`datatypes.rs#L350-L363`](https://github.com/apache/iceberg-rust/blob/86d6618804a902eac10f09c09a565aca5dad0d46/crates/iceberg/src/spec/datatypes.rs#L350-L363)).
The spec says precision must be 38 or less
([`spec.md#L273`](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/format/spec.md?plain=1#L273)).
I wrote a V2 manifest with a `decimal(36, 10)` upper bound and changed the
stored schema to `decimal(39, 10)`. `Manifest::parse_avro` succeeded, and
`Datum::to_bytes` on that bound returned `DataInvalid => PrimitiveType Decimal
must has valid precision but got 39`
([`datum.rs#L465-L469`](https://github.com/apache/iceberg-rust/blob/86d6618804a902eac10f09c09a565aca5dad0d46/crates/iceberg/src/spec/values/datum.rs#L465-L469)).
On `main`, reading `upper_bounds` on that entry panics. A Python test could
make the same change to a manifest written by PyIceberg and check that
`upper_bounds` and `lower_bounds` raise `ValueError`.
##########
bindings/python/src/manifest.rs:
##########
@@ -145,14 +146,13 @@ impl PyManifestFile {
}
#[getter]
- fn partitions(&self) -> Vec<PyFieldSummary> {
- self.inner
- .partitions
- .clone()
- .unwrap()
- .iter()
- .map(|s| PyFieldSummary { inner: s.clone() })
- .collect()
+ fn partitions(&self) -> Option<Vec<PyFieldSummary>> {
+ self.inner.partitions.as_ref().map(|partitions| {
+ partitions
+ .iter()
+ .map(|s| PyFieldSummary { inner: s.clone() })
Review Comment:
> i dont think theres coverage for `PyFieldSummary` getters either, would be
a good follow up
This PR also rewrites the `Some` branch of `partitions`, and none of the
tests in `bindings/python/tests` read a manifest list that has partition
summaries. Could `test_manifest_file_without_partition_summaries` also write a
`ManifestFile` with one `PartitionFieldSummary` and check its getters? That
would cover the rewritten branch and the `PyFieldSummary` getters here, without
needing a follow-up.
--
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]
