diff --git a/bindings/python/src/data_file.rs b/bindings/python/src/data_file.rs index b0e42e7d73..7051e83280 100644 --- a/bindings/python/src/data_file.rs +++ b/bindings/python/src/data_file.rs @@ -23,6 +23,8 @@ use pyo3::exceptions::PyValueError; use pyo3::prelude::*; use pyo3::types::PyBytes; +use crate::error::to_py_err; + #[pyclass()] pub struct PyPrimitiveLiteral { inner: PrimitiveLiteral, @@ -120,20 +122,20 @@ impl PyDataFile { } #[getter] - fn upper_bounds(&self) -> HashMap> { + fn upper_bounds(&self) -> PyResult>> { 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() } #[getter] - fn lower_bounds(&self) -> HashMap> { + fn lower_bounds(&self) -> PyResult>> { self.inner .lower_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() } diff --git a/bindings/python/src/manifest.rs b/bindings/python/src/manifest.rs index 6c042475b6..15b80c3e3e 100644 --- a/bindings/python/src/manifest.rs +++ b/bindings/python/src/manifest.rs @@ -23,6 +23,7 @@ use iceberg::spec::{ use pyo3::prelude::*; use crate::data_file::PyDataFile; +use crate::error::to_py_err; #[pyclass] pub struct PyManifest { @@ -145,14 +146,13 @@ impl PyManifestFile { } #[getter] - fn partitions(&self) -> Vec { - self.inner - .partitions - .clone() - .unwrap() - .iter() - .map(|s| PyFieldSummary { inner: s.clone() }) - .collect() + fn partitions(&self) -> Option> { + self.inner.partitions.as_ref().map(|partitions| { + partitions + .iter() + .map(|s| PyFieldSummary { inner: s.clone() }) + .collect() + }) } #[getter] @@ -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 { + Ok(PyManifest { + inner: Manifest::parse_avro(bs).map_err(to_py_err)?, + }) } #[pyclass] @@ -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 { + Ok(PyManifestList { + inner: ManifestList::parse_with_version(bs, FormatVersion::V2).map_err(to_py_err)?, + }) } pub fn register_module(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> { diff --git a/bindings/python/tests/test_manifest.py b/bindings/python/tests/test_manifest.py index e024b28086..147b81fccc 100644 --- a/bindings/python/tests/test_manifest.py +++ b/bindings/python/tests/test_manifest.py @@ -15,6 +15,7 @@ # specific language governing permissions and limitations # under the License. +from pathlib import Path from typing import Any import pytest @@ -26,7 +27,9 @@ FileFormat, ManifestEntry, ManifestEntryStatus, + ManifestFile, _manifests, + write_manifest_list, ) @@ -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 + + +@pytest.mark.parametrize( + "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) + + +@pytest.mark.parametrize( + "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