zeroshade commented on PR #1658:
URL: https://github.com/apache/iceberg-go/pull/1658#issuecomment-5220046523
Re-reviewed after `a46d30d`. This addresses the main ask from my last pass:
`WithSnapshotID` and `WithSnapshotAsOf` no longer discard each other, and the
conflict is reported in both orderings. Given that `ScanOption` is
`func(*Scan)` with no error return, recording a deferred `selectorErr` and
surfacing it at the entry points is the right shape — it keeps `Table.Scan`'s
signature intact while making the conflict unignorable. The `main` exception on
`UseRef` is documented now too.
I checked the guard coverage rather than assuming it. Every exported `Scan`
method that can produce data routes through a guarded call: `ToArrowTable` →
`ToArrowRecords` → `PlanFiles` and `ReadTasks`, and `Snapshot()` →
`ResolveSnapshot`. There is no unguarded reader, and nothing outside
`table/table.go` reads `snapshotID` or `asOfTimestamp` directly.
**One thing I would like a test for before this merges.**
Four of the six `selectorErr` guards can be deleted with `go test ./table`
still passing: the ones in `PlanFiles`, `ReadTasks`, `effectiveSchema`, and
`UseRef`. Only the `ResolveSnapshot` and `Projection` guards are exercised by
the new tests.
For local planning that is harmless — `ResolveSnapshot` is the chokepoint,
and with only its guard present every local path still errors and the suite
still passes. `effectiveSchema` reaches it at `table/scanner.go:412`, and
`PlanFiles` reaches it through `effectiveSchema`.
But `planFilesRemote` (`table/scanner.go:974-995`) never calls
`ResolveSnapshot`. It passes `SnapshotID: scan.snapshotID` straight to the
planner. `PlanFiles` does resolve as-of into a snapshot ID before the
planning-mode switch, so the as-of-first ordering is covered by that block —
but the snapshot-ID-first ordering skips it entirely and goes directly to the
remote planner.
I confirmed this with a `fakeScanPlanner` in `ScanPlanningRemote` mode,
using `WithSnapshotID(1000)` followed by `WithSnapshotAsOf(ts)`:
```
PlanFiles guard removed: PlanFiles err = <nil>
-> dispatched to planner with SnapshotID=1000,
conflict silently ignored
PR as written: PlanFiles err = invalid argument: cannot select
as-of timestamp ...
when snapshot ID 1000 is already selected
```
So that guard is the only thing standing between a conflicting scan and a
remote plan against the first selector — which is precisely the
silent-last-wins behavior this PR exists to remove, just relocated to the
remote path. It deserves a test. The harness already exists: `fakeScanPlanner`
and `fakePlanIO` in `table/scan_planning_test.go:144-163`, used the same way as
`TestScanPlanningRemoteStoresPlanIO`.
**Smaller item.** `Transaction.Scan` (`table/transaction.go:2304-2334`)
applies the same `ScanOption`s and already returns `(*Scan, error)`, but does
not surface `selectorErr`. `Table.Scan` cannot, which is the whole reason for
the deferred design — but the transaction variant can, and reporting the
conflict at construction there would be strictly better than deferring it:
```go
for _, opt := range opts {
opt(s)
}
if s.selectorErr != nil {
return nil, s.selectorErr
}
```
**Still open from my previous list**, all lower priority than the above:
non-main ref → another non-main ref, unknown-ref behavior while another
selector is active (today that reports the selector conflict rather than the
unknown ref, which I think is the better answer but is untested), and any
assertion on error content — the current tests only check `errors.Is` against
the sentinel.
**On the merge conflict.** It is mechanical. `table/time_travel_test.go` is
the only conflicted file; both sides appended test functions to the same region
and share a trailing brace, so the resolution is to keep both and close
`TestScanUseRefKeepsSnapshotSelectorsExclusive` before
`TestSnapshotAsOfWithOutOfOrderSnapshotLog`. I did that locally and the merged
tree builds, is gofmt-clean, and passes `go test ./table`. `table/scanner.go`
and `table/table.go` auto-merge, and I checked that main's change to
`scanner.go` lands inside `ResolveSnapshot`'s as-of branch, orthogonal to the
guard added at the top of that function.
CI is green on the current head (15/15).
Happy to approve once the remote-planning case has a test and
`Transaction.Scan` surfaces the error — you will need to push for the conflict
anyway.
--
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]
