Support Iceberg version travel by reference name #19111
Conversation
There was a problem hiding this comment.
The original PR was reviewed and approved by @alexjo2144 , @findinpath , and @ebyhr . I reviewed for differences, wording on user facing messages and beyond. All looks good to me.
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Show resolved
Hide resolved
| if (versionType == BIGINT) { | ||
| snapshotId = (long) version.getVersion(); | ||
| } | ||
| else if (versionType instanceof VarcharType) { |
There was a problem hiding this comment.
Do you have a plan to support CharType in follow-up?
There was a problem hiding this comment.
I did not originally, but I don't see why we wouldn't want to support it. Happy to address in a follow on though if possible. Should I create a tracking issue for follow up or do you recommend I just address it here?
There was a problem hiding this comment.
file an issue + add TODO comment here linking to it.
| @@ -784,6 +784,31 @@ public void testSnapshotReferenceSystemTable() | |||
| "('main', 'BRANCH', " + snapshotId3 + ", null, null, null)"); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
TestIcebergReadVersionedTable seems a better class for testing time travel. (No need to change this PR because it requires more work)
There was a problem hiding this comment.
Sure sounds good, when I follow up with CharType support #19111 (comment) I can move all the test in a separate commit.
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java
Show resolved
Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java
Show resolved
Hide resolved
amogh-jahagirdar
force-pushed
the
time-travel-ref
branch
from
f934e71
to
13e6c47
Compare
amogh-jahagirdar
force-pushed
the
time-travel-ref
branch
2 times, most recently
from
772fb51
to
ed66ed1
Compare
| @@ -1319,6 +1319,18 @@ SELECT * | |||
| FROM example.testdb.customer_orders FOR VERSION AS OF 8954597067493422955 | |||
| ``` | |||
|
|
|||
| Iceberg supports named references of snapshots via branches and tags. | |||
| Time travel can be performed to branches and tags in the table. | |||
| Note that when time traveling to a branch, the snapshot which is resolved is the head of the branch. | |||
There was a problem hiding this comment.
I'm a little confused by this last sentence. Could you elaborate on that?
There was a problem hiding this comment.
@amogh-jahagirdar, let's try to do a slightly condensed version of this comment to explain how our behavior is driven by a retention policy and is not a direct parallel to git branching behavior.
Later, I want to take this comment and elaborate with some visuals to explain this behavior in more detail, and we can shorten the explanation and link to the Iceberg docs at that time.
There was a problem hiding this comment.
@amogh-jahagirdar, I spoke with @ebyhr on the side and he's just gonna remove that line for now and you and I should follow up and make an easy reference point in the Iceberg docs to point to about this.
Co-authored-by: Amogh Jahagirdar <[email protected]>
ebyhr
force-pushed
the
time-travel-ref
branch
from
ed66ed1
to
a64d2ee
Compare
| if (ref == null) { | ||
| throw new TrinoException(INVALID_ARGUMENTS, "Cannot find snapshot with reference name: " + refName); | ||
| } | ||
| snapshotId = ref.snapshotId(); |
There was a problem hiding this comment.
From this implementation, it seems that there is no distinction between branch and tag queries in trino, and both are used to query a fixed snapshot ?
Description
Support Iceberg version travel by reference name.
Note: This is a rebased version of #15646. I addressed some conflicts, added an additional test, and cleaned up a nit. @jackye1995 was the original author of this PR, I just marked myself as a co-author (let me know what is the practice followed in the Trino community).
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
(x) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: