smaheshwar-pltr commented on code in PR #17735:
URL: https://github.com/apache/iceberg/pull/17735#discussion_r3946322509
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -83,10 +83,12 @@ protected ScanMetrics scanMetrics() {
protected Map<Integer, PartitionSpec> specs() {
Map<Integer, PartitionSpec> specs = table().specs();
// requires latest schema
- if (!useSnapshotSchema()
- || snapshotId() == null
- || table().currentSnapshot() == null
- || snapshotId().equals(table().currentSnapshot().snapshotId())) {
+ if (!useSnapshotSchema() || snapshotId() == null) {
+ return specs;
+ }
+
+ Snapshot currentSnapshot = table().currentSnapshot();
+ if (currentSnapshot != null &&
snapshotId().equals(currentSnapshot.snapshotId())) {
return specs;
Review Comment:
You're right that there's still a bug, but the fix isn't so straightforward
and I think warrants a larger proposal (that I've been thinking about).
Implementing this suggestion causes tests to fail. That's because there are
_two_ different snapshot-read intentions that surface as one at this point:
`scan.useSnapshot(S)` (e.g.) is used _both_ for
1. "read the table as it was at S" (so S's schema should be used for
binding), and
2. "read the table as it is now, but using S's files / data" (so the table's
current schema should be used for binding)
Spark, for example, requires both semantics (the former in a normal
time-travel read, the latter for a normal main-branch read to pin the table's
data). So, fixing the first case breaks the second, and vice-versa, when it
comes to binding.
My idea for the proper fix here is to let engines _express_ that intent
somehow, maybe by providing a schema or even something like:
```java
scan.useSnapshot(id, BindingSchema.SNAPSHOT) // read the table as it was
at id
scan.useSnapshot(id, BindingSchema.TABLE) // freeze id's files, keep
the schema
```
I'll write something up more properly here soon (I've not fleshed this out
fully, maybe I'm missing something), but I was thinking for now the current PR
would be nice to get in.
--
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]
