mbutrovich commented on PR #3260:
URL: https://github.com/apache/iceberg-rust/pull/3260#issuecomment-5876843883
> My only ask is that if we're changing the scan API, we try to do it once.
This PR and #2997 touch the same surface, so it'd be good to settle them
together before the next release.
Could we settle the builder side in this PR by keeping `as_of_time` on
`TableScanBuilder` and storing the selection in one private field?
```rust
enum SnapshotSelection {
SnapshotId(i64),
AsOfTime(i64),
}
```
The builder would hold `snapshot_selection: Option<SnapshotSelection>`, and
`snapshot_id` and `as_of_time` would each overwrite it. The builder can't
represent the conflicting state, so the error in `build()` goes away, and the
public change stays at the one new method. A branch or tag selector later
becomes another variant.
@dhruvarya-db's concern with the enum was that a second call overwrites the
first. This builder already works that way. `select`, `select_all`, and
`select_empty` all write `column_names`
([code](https://github.com/apache/iceberg-rust/blob/ee2cb05c7bed8686e24674187f200a6c5906ebcb/crates/iceberg/src/scan/mod.rs#L191-L210)),
and this PR documents last-call-wins for repeated `snapshot_id` calls and for
repeated `as_of_time` calls. Java resolves its mutually exclusive incremental
start bounds the same way. `fromSnapshotInclusive` and `fromSnapshotExclusive`
write the same `fromSnapshotId` field plus a flag, so the last call wins
([`TableScanContext`](https://github.com/apache/iceberg/blob/e689699fa6392a2da9aa10870f0d4de575ae23c1/core/src/main/java/org/apache/iceberg/TableScanContext.java#L161-L175)).
Java does reject a second snapshot selection on a table scan
([`SnapshotScan`](https://github.com/apache/iceberg/blob/e689699fa6392a2da9aa10870f0d4de575ae23c1/core/src/main/java/org/apach
e/iceberg/SnapshotScan.java#L104-L136)). Doing that here would mean changing
`snapshot_id` to return a `Result`, which breaks every caller.
As I read Java, this shape also lines up with #2997 without new
constructors. Point-in-time selection lives on the table scan (`useSnapshot`,
`useRef`, `asOfTime` in the `SnapshotScan` link above), and incremental reads
get their own entry point,
[`Table.newIncrementalAppendScan()`](https://github.com/apache/iceberg/blob/e689699fa6392a2da9aa10870f0d4de575ae23c1/api/src/main/java/org/apache/iceberg/Table.java#L71),
which matches `Table::incremental_append_scan` in #2997. Separate
`scan_as_of_time` and `scan_as_of_snapshot_id` constructors would also mean
deprecating the existing `TableScanBuilder::snapshot_id`.
--
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]
