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-4218-c78e18e737a61cca43ce26e51c21d36eb1376833 in repository https://gitbox.apache.org/repos/asf/iceberg-python.git
commit 1a47b078a3b32f2467a6a1761685cfec4749b529 Author: 龙骑士k <[email protected]> AuthorDate: Mon Nov 2 16:11:42 2026 +0000 Raise when planning a scan of an unknown snapshot (#4218) Closes issue 4197 # Rationale for this change A scan whose `snapshot_id` names no snapshot, for example an expired one, answers two ways: `plan_files()` returns no tasks and `count()` returns `0`, while `to_arrow()` on the same scan raises `ValueError: Snapshot not found: <id>`. Reproducer: - append twice, then expire the first snapshot with `t.maintenance.expire_snapshots().by_id(s1).commit()` - `t.scan(snapshot_id=s1).count()` returns `0` and `plan_files()` returns `[]` - `t.scan(snapshot_id=s1).to_arrow()` raises `ValueError: Snapshot not found: ...` The local planning path returned `[]` whenever `self.snapshot()` was `None`, which covers both an empty table and an id that is not in the metadata. Only the empty table should plan to nothing. Java makes the same distinction. `SnapshotScan.useSnapshot` rejects an unknown id with `Preconditions.checkArgument(table().snapshot(scanSnapshotId) != null, "Cannot find snapshot with ID %s", scanSnapshotId)`, so the `snapshot == null` branch in `SnapshotScan.planFiles()` only returns an empty result for an empty table. ### What changed | Where | Now | | --- | --- | | `DataScan._plan_files_local` | Raises `ValueError(f"Snapshot not found: {self.snapshot_id}")` when a `snapshot_id` is set but not found, the same error that `projection()` and therefore `to_arrow()` already raise | | `count()` | Uses `plan_files()`, so it raises as well | | `to_arrow()`, `to_arrow_batch_reader()` | Already raised from `projection()`, unchanged | | A scan without a `snapshot_id` on a table without snapshots | Still plans to nothing | ### Why planning, not the scan constructor Java checks when the snapshot is set on the scan; this change checks when the files are planned. pyiceberg accepts the id in the `DataScan` constructor and in `update()`, and `TableScan.snapshot()` returns `None` for an unknown id, which `test_table_scan_missing_snapshot_zero` relies on. Checking in planning keeps those as they are and raises at the same point as `to_arrow()`, which only fails once it is called. ### Server-side planning Unchanged: the snapshot id is sent to the REST server, which plans against its own metadata. The Java reference implementation calls `useSnapshot` in `CatalogHandlers.planTableScan`, so it rejects an unknown id with a 400 that pyiceberg raises as `BadRequestError`. With server-side planning, `plan_files()` and `count()` therefore raise the server's error when the server rejects the id, instead of the `ValueError` that `to_arrow()` raises from `projection()` before planning (・ω・)b ## Are these changes tested? Yes, in `tests/table/test_init.py`: | Test | Checks | | --- | --- | | `test_table_scan_plan_files_missing_snapshot` | `plan_files()` and `count()` raise `Snapshot not found: 0` for a scan of `table_v2` with snapshot id `0`, which the fixture does not have. The id `0` also makes sure the check does not treat the id as a boolean. Without this change, the test fails with `DID NOT RAISE ValueError` | | `test_data_scan_plan_files_no_current_snapshot` | A table without snapshots is already covered: `plan_files()` returns `[]` and `count()` returns `0` for a scan without a `snapshot_id` of a table without snapshots | ## Are there any user-facing changes? Yes, a bug fix: `plan_files()` and `count()` on a scan whose `snapshot_id` names no snapshot now raise `ValueError: Snapshot not found: <id>`, instead of returning no tasks and `0` (｀・ω・´) ## AI Disclosure Human-led contribution. Grok Bot, working for dragonknightk, assisted with this change. All submitted content was reviewed by a human before submission. --- pyiceberg/table/__init__.py | 2 ++ tests/table/test_init.py | 11 +++++++++++ 2 files changed, 13 insertions(+) diff --git a/pyiceberg/table/__init__.py b/pyiceberg/table/__init__.py index 8d79472af..73c435f07 100644 --- a/pyiceberg/table/__init__.py +++ b/pyiceberg/table/__init__.py @@ -2490,6 +2490,8 @@ class DataScan(TableScan): """Plan files locally by reading manifests.""" snapshot = self.snapshot() if not snapshot: + if self.snapshot_id is not None: + raise ValueError(f"Snapshot not found: {self.snapshot_id}") return [] return self._manifest_planner.plan_files(snapshot.manifests(self.io)) diff --git a/tests/table/test_init.py b/tests/table/test_init.py index f6b984f1a..ff7ecd7c3 100644 --- a/tests/table/test_init.py +++ b/tests/table/test_init.py @@ -314,6 +314,17 @@ def test_table_scan_missing_snapshot_zero(table_v2: Table) -> None: assert table_v2.scan(snapshot_id=0).snapshot() is None +def test_table_scan_plan_files_missing_snapshot(table_v2: Table) -> None: + # table_v2 has no snapshot with id 0, so planning raises the same error as projection() and to_arrow() + scan = table_v2.scan(snapshot_id=0) + + with pytest.raises(ValueError, match="Snapshot not found: 0"): + scan.plan_files() + + with pytest.raises(ValueError, match="Snapshot not found: 0"): + scan.count() + + @pytest.mark.parametrize("snapshot_id", [0, 3051729675574597004]) def test_table_scan_ref_cannot_override_snapshot(table_v2: Table, snapshot_id: int) -> None: with pytest.raises(ValueError, match=f"Cannot override ref, already set snapshot id={snapshot_id}"):
