This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-rust.git
The following commit(s) were added to refs/heads/main by this push:
new 8cb2adedd fix(python): raise instead of panicking in the manifest
bindings (#3241)
8cb2adedd is described below
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
