This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-3241-5a91091c53a5dfebcc8f85f78f0eeedb269d0b87 in repository https://gitbox.apache.org/repos/asf/iceberg-rust.git
commit 8cb2adeddb4f8da1ca7bd86ca303337f011c12e2 Author: Alex Stephen <[email protected]> AuthorDate: Mon Sep 28 18:41:04 2026 +0000 fix(python): raise instead of panicking in the manifest bindings (#3241) There's some assorted places that we need to raise errors instead of panicking. --- bindings/python/src/data_file.rs | 10 +++--- bindings/python/src/manifest.rs | 33 +++++++++--------- bindings/python/tests/test_manifest.py | 64 ++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 21 deletions(-) diff --git a/bindings/python/src/data_file.rs b/bindings/python/src/data_file.rs index b0e42e7d7..7051e8328 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<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() } #[getter] - fn lower_bounds(&self) -> HashMap<i32, Vec<u8>> { + fn lower_bounds(&self) -> PyResult<HashMap<i32, Vec<u8>>> { 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 6c042475b..15b80c3e3 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<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() + }) } #[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<PyManifest> { + 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<PyManifestList> { + 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 e024b2808..147b81fcc 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 @@ from pyiceberg.manifest import ( 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 + + [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
