Conversation
Reviewer's GuideFix Iceberg time-travel reads to resolve columns from the schema recorded on the selected historical snapshot, including version expressions, while preserving current-schema semantics for plain and branch reads. The analyzer now honors versioned handles, and regression tests cover schema evolution, refs, and delete protection. Sequence diagram for Iceberg historical schema resolutionsequenceDiagram
participant Query as Query
participant Analyzer as StatementAnalyzer
participant Metadata as IcebergMetadata
participant Table as IcebergTable
participant Snapshot as SnapshotSchema
Query->>Analyzer: visitTable
Analyzer->>Metadata: getTableHandle
Metadata->>Table: readsHistoricalSnapshot
alt historical snapshot
Metadata->>Snapshot: SnapshotUtil.schemaFor
Snapshot-->>Metadata: snapshot schema
else plain read or branch
Table-->>Metadata: current schema
end
Metadata-->>Analyzer: versioned table handle
Analyzer->>Metadata: getTableMetadata(handle)
Analyzer->>Metadata: getColumnHandles(handle)
Metadata-->>Analyzer: columns from selected schema
Flow diagram for Iceberg read schema selectionflowchart TD
A[Read table] --> B{Snapshot selected?}
B -->|No| C[Use current schema]
B -->|Yes| D{Historical snapshot?}
D -->|Yes: snapshot, timestamp, or tag| E[Use schema recorded on snapshot]
D -->|No: live branch| C
E --> F[Resolve columns from versioned table handle]
C --> G[Resolve columns from current table schema]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
A query can pick one old snapshot in two ways:
SELECT ... FROM t FOR VERSION AS OF 123
SELECT ... FROM "t@123"
IcebergTableHandle.isSnapshotSpecified says whether a read picked one snapshot.
It was set only for the second way, so the first way claimed it was reading the
table's newest data.
The flag is needed because the snapshot id cannot answer that question on its
own. For a plain read, resolveSnapshotIdByName fills in the current snapshot id,
so both kinds of read carry one, and only this flag tells them apart.
No user query misbehaves today. Three methods read the flag -- tableExecute,
beginDelete and supportsMetadataDelete -- and all three refuse to change a table
through a read of one old snapshot. DELETE FROM "t@123" reaches them and is
refused. DELETE ... FOR VERSION AS OF cannot be written at all, because the
grammar attaches AS OF only to a table you select from. tableExecute has no
caller anywhere.
It still has to be fixed, because reading an old snapshot's schema depends on it.
Iceberg keeps data history and schema apart: a snapshot is a set of data files,
while the schema lives in the table metadata. ALTER TABLE writes new table
metadata and makes no new snapshot. So after
CREATE TABLE t (a int, b int);
INSERT INTO t VALUES (1, 2); -- snapshot 123, columns (a, b)
ALTER TABLE t DROP COLUMN b; -- new schema, still only snapshot 123
both of these resolve to snapshot 123:
SELECT * FROM t -- must return (1)
SELECT * FROM t FOR VERSION AS OF 123 -- must return (1, 2)
Same snapshot id, two different correct answers, so the snapshot id cannot tell
them apart. Only this flag can. The next commit uses it to fix that read.
The test calls beginDelete and supportsMetadataDelete directly, because no DELETE
statement can reach them with AS OF.
3b4a0a2 to
039c8aa
Compare
yingsu00
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
Explicit changelog reads can receive inconsistent historical metadata and current-schema column handles after schema evolution.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes #23553 by resolving Iceberg time-travel columns from the selected snapshot’s schema.
Changes:
- Re-fetches metadata using versioned table handles.
- Stores snapshot-appropriate schemas while preserving branch behavior.
- Adds schema-evolution and mutation-safety regression coverage.
File summaries
| File | Description |
|---|---|
StatementAnalyzer.java |
Resolves columns from versioned handles. |
IcebergUtil.java |
Selects current or snapshot schema. |
IcebergAbstractMetadata.java |
Propagates read schemas through Iceberg metadata. |
TestIcebergTableVersion.java |
Tests schema evolution and delete safeguards. |
IcebergDistributedTestBase.java |
Updates branch/tag expectations. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // filesystem metadata will fail. | ||
| // See https://github.com/prestodb/presto/pull/21181 | ||
| Optional<Schema> tableSchema = tryGetSchema(table); | ||
| Optional<Schema> tableSchema = tryGetReadSchema(table, readsHistoricalSnapshot(table, tableVersion, name), tableSnapshotId); |
There was a problem hiding this comment.
Good catch, thank you - this was a real inconsistency, so a changelog read is now left on the current schema.
tryGetReadSchema returns the current schema when the table type is CHANGELOG, so the handle a changelog read carries reports the same schema its column handles (getColumnHandles) and its splits (ChangelogSplitSource) are built from. Changing all the changelog paths to the historical schema would be a larger change than this fix, and it is not needed for the bug being fixed here.
Added TestIcebergTableChangelog.testChangelogOfAnOldSnapshotUsesTheCurrentSchema, which drops a column after a snapshot and then checks SHOW COLUMNS FROM "t@<old snapshot>$changelog" reports rowdata as row("a" integer), and that the changelog is still readable. Without the CHANGELOG case the new test fails, so the behaviour is now pinned.
| * is being read. | ||
| * | ||
| * The question used to be answered correctly by the handle for "t@123" but not for | ||
| * FOR VERSION AS OF, so the second way was refused while the first way was not. |
There was a problem hiding this comment.
Thanks - the order in that sentence was right, but only if the reader maps "the second way" back to the order the two ways were listed in above, which is too easy to lose. It now names them:
The question used to be answered correctly by the handle for "t@123" but not for FOR VERSION AS OF, so a delete through "t@123" was refused while one through FOR VERSION AS OF was not.
Which matches the old behaviour: snapshotSpecified was set only for the "t@123" form, so a delete through it was refused, while FOR VERSION AS OF was not refused.
imjalpreet
left a comment
imjalpreet
left a comment
There was a problem hiding this comment.
Thanks, @yingsu00.
Could you please take a look at one of the suggestions from Copilot: https://github.com/prestodb/presto/pull/28503/changes#r4050590794?
Apart from that I have added a couple of minor suggestions.
Also, I feel we should probably update the documentation as well. We could add which schema would be used for Time Travel and which schema would be used for a branch. Do you think it makes sense?
Since this PR also includes changes in the analyzer maybe we can update the PR title scope to -> fix(analyzer, iceberg): ...
| ConnectorTableVersion version = tableVersion.get(); | ||
| if (version.getVersionType() == VersionType.VERSION && version.getVersionExpressionType() instanceof VarcharType) { | ||
| SnapshotRef ref = table.refs().get(((Slice) version.getTableVersion()).toStringUtf8()); | ||
| return ref != null && ref.isTag(); | ||
| } |
There was a problem hiding this comment.
Suggestion from my side: I think we could reuse an Iceberg utility method for this case, as Iceberg already implements the tag/branch rule via SnapshotUtil.schemaFor(Table, String ref).
But this would require a bit of a refactor to combine readsHistoricalSnapshot + tryGetReadSchema into one schema-returning method handling all the cases.
There was a problem hiding this comment.
Done, thank you - this reads much better, and it removes our own copy of Iceberg's rule.
readsHistoricalSnapshot is gone, and IcebergUtil.tryGetReadSchema(Table, IcebergTableName, Optional<ConnectorTableVersion>, Optional<Long>) now returns the schema for every case:
FOR SYSTEM_VERSION AS OF '<ref>'->SnapshotUtil.schemaFor(table, ref), which returns the schema of the tag's snapshot for a tag, and the current schema for a branch or an unknown name- a snapshot id or a timestamp (
"t@123",FOR VERSION AS OF <id>,FOR TIMESTAMP AS OF) ->SnapshotUtil.schemaFor(table, snapshotId) - a plain read,
"t.branch_x", and a changelog -> the current schema
testRefsTable still asserts that a tag read sees the dropped column while a branch read does not, so the tag/branch rule stays covered end to end.
| return Optional.ofNullable(table.schema()); | ||
| } | ||
| catch (TableNotFoundException e) { | ||
| log.warn(String.format("Unable to fetch schema for table %s: %s", table.name(), e.getMessage())); | ||
| return Optional.empty(); | ||
| } |
There was a problem hiding this comment.
nit: duplicate, we could delegate to the IcebergUtil.tryGetSchema method just above this
There was a problem hiding this comment.
Done - every case that wants the current schema now returns tryGetSchema(table), so the table.schema() call and its TableNotFoundException handling are no longer duplicated. What is left is one small helper for the two SnapshotUtil.schemaFor calls, which need the same logging and fallback.
The columns of a time travel query were resolved from the table's
current schema, so the wrong columns were returned when an old snapshot
was asked for:
CREATE TABLE t (a int, b int);
INSERT INTO t VALUES (1, 2); -- snapshot 123
ALTER TABLE t DROP COLUMN b;
SELECT * FROM t FOR VERSION AS OF 123 -- returned (1), should be (1, 2)
SELECT b FROM t FOR VERSION AS OF 123 -- "Column 'b' cannot be resolved"
Column b's values are still held in that snapshot's data files, and
time travel is how they are read, so this is the case that matters
most. An added column was the mirror problem: a column that the old
snapshot never had was shown, filled with nulls. A renamed column was
resolved by its new name at a snapshot where only the old name was
known.
Fixes prestodb#23553
Data history and schema are kept apart by Iceberg. A snapshot is a set
of data files, the schema is held in the table metadata, and the schema
a snapshot was written with is recorded on it. New table metadata is
written by ALTER TABLE and no new snapshot is made, so the newest
snapshot and the current schema can disagree.
This commit contains two layers of fixes:
In the analyzer, the column list was taken from getTableColumnsMetadata,
where the table is looked up by name alone, so the handle it builds
carries no version. The versioned handle was built separately and used
only for reading, so the connector was never asked which columns that
version has. The columns are now re-asked through the versioned handle
in visitTable. This part is connector-agnostic: any connector where a
schema is recorded per version is helped by it.
In the Iceberg connector, the read schema is now taken from the
snapshot by Iceberg's own SnapshotUtil.schemaFor, and the current schema
is used as a fallback for a snapshot that recorded none. The schema is
decided once in getTableHandle and stored in the handle's
tableSchemaJson, from where it is reused by getColumnHandles and
getTableMetadata instead of being worked out again.
Branches and tags are treated differently here, as Iceberg requires. A
point in history is marked by a tag, so the schema of its snapshot is
used. A branch is still being written to, so the current schema is
used. Either can be named by FOR SYSTEM_VERSION AS OF and both are
resolved to a snapshot id, so the two are told apart by Iceberg's own
SnapshotUtil.schemaFor(Table, String), which applies that rule itself.
A changelog is left on the current schema. Its rows are reported as one
rowdata column, and that column, its column handles and its splits are
all built from the current schema, so reporting the schema of an old
snapshot would advertise a column that is then not read.
The previous commit is needed for this. After a schema change that made
no snapshot, one snapshot id can be reached both by a plain read and by
a pinned read, and different schemas are still needed, so the choice
cannot be made from the snapshot id alone.
The old behaviour was asserted by testRefsTable, where this bug was
cited. It is now asserted that the dropped column is seen when a tag is
read and is not seen when a branch is read.
Which schema each kind of read uses is now written down in the Iceberg
connector documentation.
yingsu00
changed the title
039c8aa to
86a4175
Compare
|
Thanks @imjalpreet - all four are addressed, and the PR title scope is updated to
Tests: |
Description
This PR contains two commits:
IcebergTableHandle.isSnapshotSpecifiedfor a read that picked a snapshot with a version expression, not only for the"t@123"table-name form.Resolves #23553
Motivation and Context
This fix is a pre-requisite of the upcoming plan cache for Iceberg.
Iceberg keeps data history and schema apart. A snapshot is a set of data files; the schema lives in the table metadata, and each snapshot records which schema it was written with.
ALTER TABLEwrites new table metadata and creates no new snapshot, so the newest snapshot and the current schema can disagree.Presto resolved a time travel query's columns from the current schema, which is wrong three different ways:
DROP COLUMN bbmissing;SELECT bfails analysis(1, 2);SELECT breturns2ADD COLUMN cc, all nulls(1);SELECT cfails analysisRENAME b TO b_renamedThe dropped-column case is the one that matters most:
b's values are still in that snapshot's data files, and time travel is how you would go back and read them.The fix needed two layers, and the connector alone is not enough
The issue suggests fixing
IcebergUtil. That is necessary but not sufficient, and I confirmed that a connector-only change has no observable effect. The newly added regression tests still failed.In
StatementAnalyzer.visitTable, the column list came fromgetTableColumnsMetadata(...), which looks the table up by name and therefore builds a handle with no version on it (MetadataUtils.getTableColumnMetadatacallsmetadataResolver.getTableHandle(tableName)). The versioned handle was built separately on the next line and used only for reading. So no matter what the connector reported for a versioned handle, the query's field list came from the current schema.The analyzer change is connector-agnostic, meaning that any connector that records a schema per version benefits.
Branches and tags are not the same here
FOR SYSTEM_VERSION AS OF '<name>'accepts a branch or a tag and resolves both to a snapshot id, discarding which it was. Iceberg treats them differently (schema selection with branches and tags):readsHistoricalSnapshotlooks the name up intable.refs()to tell them apart. Treating every named version as historical breaks branch reads, which is what caught my first attempt.Why commit 1 is a prerequisite
After a schema change that created no snapshot, a plain read and a pinned read can resolve to the same snapshot id and still need different schemas —
resolveSnapshotIdByNamefills in the current snapshot id for a plain read. So the snapshot id alone cannot decide which schema to use; only a flag saying "this query picked a snapshot itself" can. That flag was wrong forFOR VERSION AS OF.Impact
Time travel reads now see the schema their snapshot was written with. Behaviour changes for
FOR VERSION AS OF,FOR VERSION BEFORE,FOR TIMESTAMP AS OF/BEFORE, the"table@snapshotId"name form, andFOR SYSTEM_VERSION AS OF '<tag>'.Unchanged: plain reads,
FOR SYSTEM_VERSION AS OF '<branch>', and"table.branch_x"all continue to use the current schema.IcebergDistributedTestBase.testRefsTableasserted the old behaviour and cited this issue in a comment; it now asserts that reading a tag sees the dropped column while reading a branch does not.Test Plan
New and updated tests:
TestIcebergTableVersion#testTableVersionWithSchemaEvolution— covers drop, add and rename against a snapshot, and asserts thatALTER TABLEcreates no new snapshot (so the pinned read and the plain read land on the same snapshot id). Fails on master.TestIcebergTableVersion#testDeleteIsRefusedForOldSnapshot— assertsbeginDeletethrows andsupportsMetadataDeletereturns false for all four ways of picking a snapshot. It calls the two methods directly because the grammar attachesAS OFonly to a table you select from, so noDELETEstatement can reach them. Fails on master for the version-expression cases and passes for the"t@123"case, which is the asymmetry commit 1 fixes.IcebergDistributedTestBase#testRefsTable— updated as described above.Contributor checklist
Release Notes
Summary by Sourcery
Make Iceberg time travel reads use the schema associated with the selected snapshot while preserving current-schema behavior for non-historical reads.
Bug Fixes:
Enhancements:
Documentation:
Tests: