From 8a0e4fa3976091f8b12060c937e594bd3f47e066 Mon Sep 17 00:00:00 2001
From: KK <37928252+dragonknightk@users.noreply.github.com>
Date: Fri, 30 Oct 2026 23:27:20 +0800
Subject: [PATCH] Raise when planning a scan of an unknown snapshot
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 8d79472af5..73c435f07f 100644
--- a/pyiceberg/table/__init__.py
+++ b/pyiceberg/table/__init__.py
@@ -2490,6 +2490,8 @@ def _plan_files_local(self) -> Iterable[FileScanTask]:
         """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 f6b984f1a3..ff7ecd7c37 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}"):