kevinjqliu commented on code in PR #3241:
URL: https://github.com/apache/iceberg-rust/pull/3241#discussion_r4125590971
##########
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)
+
+
[email protected](
+ "payload", [b"", b"not-an-avro-file",
b"Obj\x01truncated-after-the-avro-magic"]
+)
+def test_read_manifest_list_raises_on_invalid_avro(payload: bytes) -> None:
+ from pyiceberg_core import manifest
+
+ with pytest.raises(ValueError):
+ manifest.read_manifest_list(payload)
Review Comment:
👍
##########
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
##########
bindings/python/src/manifest.rs:
##########
@@ -221,10 +220,10 @@ impl PyManifestList {
}
#[pyfunction]
-pub fn read_manifest_list(bs: &[u8]) -> PyManifestList {
- PyManifestList {
- inner: ManifestList::parse_with_version(bs,
FormatVersion::V2).unwrap(),
- }
+pub fn read_manifest_list(bs: &[u8]) -> PyResult<PyManifestList> {
+ Ok(PyManifestList {
+ inner: ManifestList::parse_with_version(bs,
FormatVersion::V2).map_err(to_py_err)?,
Review Comment:
we might want to follow up and parameterize this one 😄
##########
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
##########
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)
+
+
[email protected](
+ "payload", [b"", b"not-an-avro-file",
b"Obj\x01truncated-after-the-avro-magic"]
+)
+def test_read_manifest_list_raises_on_invalid_avro(payload: bytes) -> None:
+ from pyiceberg_core import manifest
+
+ with pytest.raises(ValueError):
+ manifest.read_manifest_list(payload)
+
+
+def test_manifest_file_without_partition_summaries(tmp_path: Path) -> None:
+ """Field summaries are optional, so a manifest list without them reads
back as None."""
+ from pyiceberg_core import manifest
+
+ manifest_list_file = str(tmp_path / "snap.avro")
+ io = PyArrowFileIO()
+
+ with write_manifest_list(
+ format_version=2,
+ output_file=io.new_output(manifest_list_file),
+ snapshot_id=25,
+ parent_snapshot_id=None,
+ sequence_number=1,
+ avro_compression="null",
+ ) as writer:
+ writer.add_manifests(
+ [
+ ManifestFile.from_args(
+ manifest_path="s3://bucket/metadata/manifest.avro",
+ manifest_length=1024,
+ partition_spec_id=0,
+ added_snapshot_id=25,
+ sequence_number=1,
+ min_sequence_number=1,
+ added_files_count=1,
+ existing_files_count=0,
+ deleted_files_count=0,
+ added_rows_count=10,
+ existing_rows_count=0,
+ deleted_rows_count=0,
+ partitions=None,
+ )
+ ]
+ )
+
+ bs = io.new_input(manifest_list_file).open().read()
+ manifest_file = manifest.read_manifest_list(bs).entries()[0]
+
+ assert manifest_file.partitions is None
Review Comment:
I think `None` is correct here, and not empty list `[]`.
https://github.com/apache/iceberg-rust/pull/2886#discussion_r3860287765 called
this out too
--
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]
