dragonknightk opened a new pull request, #4218:
URL: https://github.com/apache/iceberg-python/pull/4218
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
