Copilot commented on code in PR #3241:
URL: https://github.com/apache/iceberg-rust/pull/3241#discussion_r4125704799
##########
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>>> {
self.inner
.upper_bounds()
.iter()
- .map(|(k, v)| (*k, v.to_bytes().unwrap().to_vec()))
+ .map(|(k, v)| Ok((*k, v.to_bytes().map_err(to_py_err)?.to_vec())))
.collect()
}
Review Comment:
Now that bounds serialization failures are surfaced to Python (good), please
ensure `to_py_err` yields an exception with enough context for users to
diagnose which field/bound failed (e.g., include whether it was upper/lower
bounds and the column id). Without that context, failures raised from a
property getter can be hard to debug.
##########
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() })
+ .collect()
+ })
}
Review Comment:
The PR description says it changes the type signature of
`PyManifest.partitions()` to allow `None`, but the code change here is for
`PyManifestFile`'s `partitions` getter. Please align the PR description with
the actual API being changed (or update the code if the intent was to change
`PyManifest.partitions`).
##########
bindings/python/tests/test_manifest.py:
##########
@@ -171,3 +174,64 @@ def
test_read_manifest_entry(generated_manifest_entry_file: str) -> None:
assert data_file.split_offsets == [4]
assert data_file.equality_ids is None
assert data_file.sort_order_id == 0
+
+
[email protected](
+ "payload", [b"", b"not-an-avro-file",
b"Obj\x01truncated-after-the-avro-magic"]
+)
+def test_read_manifest_entries_raises_on_invalid_avro(payload: bytes) -> None:
+ from pyiceberg_core import manifest
+
+ with pytest.raises(ValueError):
+ manifest.read_manifest_entries(payload)
Review Comment:
The same invalid-payload list is duplicated for both manifest-entries and
manifest-list tests. Consider extracting it into a shared module-level constant
to avoid divergence if the test corpus changes.
--
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]
