[dragonknightk]

Closes #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.

[dragonknightk]

@RyanLin5967 thanks for flagging this one — a count() of 0 looks like an answer, so nothing points you at a snapshot that is simply gone (๑•̀ㅂ•́)و✧

The fix is tiny: two lines in _plan_files_local plus one test, +13 / −0 across two files. plan_files() and count() now raise Snapshot not found when the scan names an id that is not in the metadata, which is what to_arrow() already does. An empty table still plans to nothing, and the test pins id 0 so the check cannot be fooled by a falsy id.

One check is red and not from this PR: Run zizmor flags pypa/gh-action-pypi-publish@v1.14.2 in nightly-pypi-build.yml, a file this change does not touch. #4216 bumps it to v1.14.3, so this should go green once that lands.

@Fokko ready for review when you have a moment (´｡• ᵕ •｡`)

[Fokko]

Sounds reasonable to fail when a snapshot-id is missing