Spec: Add snapshot tagging and branching

[jackye1995]

No description provided.

[jackye1995]

@rdblue @nastra @szehon-ho

[ajantha-bhat]

Suggested change

	A andD cannot be expired due to branch b1 policy, so only F and G are expired.
	A and D cannot be expired due to branch b1 policy, so only F and G are expired.

[ajantha-bhat]

I am also bit confused about what happens after the expiry? we delete F & G snapshots and also drop branch b2 (we remove b2 from table metadata?)

[jackye1995]

If a snapshot with reference is dropped, the reference is also dropped, so branch b2 will be removed.

[ajantha-bhat]

Is it better to have some snapshots in the example that are not referenced by branches and tags just to apply global retention policy ? now all snapshots are under branch or tag. so global policy cannot be applied. (This is based on my assumption, please see last comment for this page)

[jackye1995]

sure, I will try to add one

[ajantha-bhat]

Do we really need to honour the global policy when that branch is having local policy ? Is it better to delete A and B in this case as local policy says min snapshots to keep is 1 ?
I think applying global policies is good enough only for snapshots that are not involved in any branch, tag. WDYT ?

[jackye1995]

Yes, based on the mechanism described to determine snapshots to expire, we are determining which snapshots are not removed after evaluating all the related policies, so it would eventually hit the global policy and stop there.

I think it also fits the meaning of "minimum number of snapshots to keep", because it's the minimum, not that we need to always reach that value.

[rdblue]

Another reason to avoid saying "global policy" is that it isn't clear whether branches override "global" or whether there is some way to merge them (like max(table-min-snapshots, branch-min-snapshots)).

I think calling the "global" settings "table defaults" is a good way to help people understand.

[ajantha-bhat]

Do we need to keep each branch's latest snapshot id here ? I wonder how do we support query on each branch or tag ?

[jackye1995]

the refs always record the latest snapshot ID of a branch, query of branch and tag will use that information.

[rdblue]

As I noted above, I think that we should remove this. The "current branch" must always be main.

That simplifies a couple things in the spec:

• If refs is present, it must contain a main branch
• The main branch reference never expires
• No need for current-branch
• current-snapshot-id must always match the snapshot ID of the main branch

[harshm-dev]

Since Iceberg does not plan to offer Git-like merge, can we assume cherry-pick can happen only in a scenario similar to fast-forward merges (where there's no additional commit on the base)?
If yes, do you think it is worth mentioning this pre-requisite?

[jackye1995]

I would imagine it to be similar to the Spark procedure cherrypick_snapshot. It does not have any requirement, you need to know exactly what you are doing to do the cherry-pick.

[harshm-dev]

How do we mention the branch name in the property?

[jackye1995]

An update API will handle the actual update operation, which will allow specifying a branch name. It is not a part of this doc.

[harshm-dev]

Is it possible to have an enum?

[rdblue]

This is going to be serialized in JSON, so there isn't an enum type. In addition, enums are a minefield for schema evolution, so it's usually better to use a string so that people understand there may be other (unsupported) values.

[harshm-dev]

Will it make sense to mention the default, since it is an optional field.

[jackye1995]

good point, added

[harshm-dev]

Is the name required to be unique? If yes, will it be worth mentioning the same?

[rdblue]

Yes, it should be unique and I agree that it should be called out. We could also make this a map to guarantee it in JSON.

[harshm-dev]

Should we have a current_branch in metadata? Will it make more sense to position this as a client-session specific conf?

[jackye1995]

I think the metadata need to know this information, because it will use the current branch to know what is the default branch to add the next commit.

[omarsmak]

I am bit lost here, I thought we will there will be as well a property to disable the retention either on snapshot or globally?

[ajantha-bhat]

we already have GC_ENABLED table property for that.
more details here: #3246

[rdblue]

I think that "global" might be confusing. How about "Table level retention policy" and "Branch level retention policy"? Reference retention (max-ref-age-ms) can also be a separate section.

[omarsmak]

I am wondering here about offers user a Git-like experience, since we don't intend to offer a full fledged git like experience, does it make sense to add limited Git-like experience? In order not to confuse users I guess, WDYT?

[jackye1995]

Yes I agree it's probably not good to say Git-like given it's not completely Git-like. But limited sounds a bit too strong to me, I am changing the wording to the following:

Iceberg snapshot tagging and branching feature provides users more functionalities in managing Iceberg snapshot lifecycle, and allows users to treat table snapshots just like git branch and tag.
Users can assign tags to snapshots, create new branches, set the current branch for read and write, and configure customized retention policy for them.

Please let me know what you think.

[omarsmak]

Here a branch is created when the user configure current-branch property?

[jackye1995]

no, user set a snapshot as a BRANCH type reference to create a branch. Current branch is used as the default branch to read the latest table snapshot and write the next commit.

[nastra]

I think mentioning a git-like experience might rather be confusing for users. To me this feature is rather Named Snapshots (or something similar)

[nastra]

maybe change to The duration before a snapshot reference can be expired by any automatic snapshot expiration process

[rdblue]

I don't think this should be required. Why not default to the table setting?

[jackye1995]

@nastra The issue with The duration before a snapshot reference can be expired by any automatic snapshot expiration process is that it does not express the fact that all snapshots in a branch would be affected by the policy. But I agree the current wording is a bit convoluted, any suggestion would be welcome.

[jackye1995]

@rdblue good point, I was defaulting to the table default by setting it to 5 days explicitly so in the spec it is required.

I agree it sounds better to inherit the table's latest value of history.expire.max-snapshot-age-ms, although the retention would change if user updates the table property, that is likely the expected behavior, similar to how metrics mode is configured for the whole table and inherited for each column. I will make the change.

[nastra]

I agree that the git-like experience part could confuse users as to what that really means.

I believe in some version of the design doc you named it Named Snapshots (or something similar) as to me that's what this feature really is. Those are named snapshots where the type can be either a branch (moves forward with changes) or a tag (does not move forward) and I would probably rather explain this feature from this perspective

[jackye1995]

Agree, I updated it, please let me know what you think about the new wording.

[rymurr]

Hey @jackye1995 I left a lot of comments on the google doc. is that still the referecne or should I copy the comments here?

[rdblue]

I would avoid linking to user experience from the spec. The spec sets requirements and should include enough to understand those requirements. But we don't want to be overly prescriptive or make it seem like there are additional requirements outside of the spec.

[rdblue]

I think you can make this introduction a bit more clear. Something like this:

Iceberg tables keep track of branches and tags using snapshot references. Tags are labels for individual snapshots. Branches are mutable named references that can be updated by committing a new snapshot as the branch's referenced snapshot using the [Commit Conflict Resolution and Retry] procedures.

[jackye1995]

sounds good, updated

[rdblue]

I don't think this change is necessary.

[jackye1995]

removed

[rdblue]

I think that this PR needs to address compatibility with current-snapshot-id. For the duration of v2, writers must still produce current-snapshot-id.

[jackye1995]

I see the current-snapshot-id field is described as optional, but when checking the TableMetadataParser it looks like we always assume the field exists for both v1 and v2.

https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/TableMetadataParser.java#L400

Is that an issue with the spec itself?

If this field is required for v1 and v2, then I think there is no concern? Or are you suggesting adding a new column for format v3 which deprecates current-snapshot-id?

[rdblue]

The current snapshot ID is optional because the table may not have a snapshot ID. We can clarify this in the spec and update the parser to return null.

[jackye1995]

Sorry for the delayed reply @rymurr @nastra @omarsmak @harshm-dev @ajantha-bhat

I have updated based on the comments and also replied questions in the design doc, please let me know if there is any further question.

[rdblue]

After the confusion over current-branch, I think that the current branch should always be main so that people don't try to "checkout" branches in a table. That's always an incorrect operation.

In that case, this should be the ID of the current table snapshot; must be the same as the current ID of the main branch.

[rdblue]

I think it has proven too confusing to have that feature. Plus, we don't really need it if we support renaming branch refs. Making another branch main is as easy as this:

table.updateReferences()
    .rename("main", "main_v1")
    .rename("dev", "main")
    .commit();

[rdblue]

I think that both of these only apply to branches because these control the snapshots that are kept behind the branch's current snapshot.

We also need a new configuration option at the table level and at the reference level that controls when the reference itself is removed. That is, if the referenced snapshot is older than some interval, we remove the reference itself. That helps prune old branches and tags that are no longer used. We'll probably default it to keep them forever, but it is good to have the setting for compliance needs. For that, how about max-ref-age-ms and history.expire.max-ref-age-ms?

[rdblue]

It isn't clear from the spec how this would be done, especially without the max-ref-age-ms property.

[jackye1995]

My original thinking is that if the snapshot is expired and it has reference, the reference is removed at the same time. But I agree max-ref-age-ms does give more flexibility, I will update based on that.

[rdblue]

Nit: it's unclear who "we" is in documentation. It's more direct and clear to say "Iceberg uses"

[jackye1995]

I will remove all the usages of we

[rdblue]

I think it would be better to say something like "Table snapshots expire and are removed from metadata to allow removed or replaced data files to be physically deleted. The snapshot expiration procedure removes snapshots from table metadata and applies the table's retention policy. Retention policy ..."

[rdblue]

If retention properties are in the spec, then this section should be in the spec as well.

[rdblue]

Nit: Could you use sentence case? Also, there are two "4." points. While markdown will fix it, I like to keep them updated.

[rdblue]

I find this description of the process confusing. Why build a pool and remove, rather than building a set of snapshots to retain? I also don't think it makes sense to refer to applying global policy as a separate step or "choosing" snapshots from the pool. Instead, I think it is more clear to use a procedure that focuses on the snapshots that must be kept:

1. Start with an empty set of snapshots to retain
2. Remove any refs (except main) where the referenced snapshot is older than max-ref-age-ms
3. For each branch and tag, add the referenced snapshot to the retained set
4. For each branch, add its ancestors to the retained set until:
   • The snapshot is older than max-snapshot-age-ms, AND
   • The snapshot is not one of the first min-snapshots-to-keep in the branch (including the branch's referenced snapshot)
5. Expire any snapshot not in the set of snapshots to retain.

Note: max-ref-age-ms, max-snapshot-age-ms, and min-snapshots-to-keep can be set in ref metadata. If not set, the ref should use default values from table metadata.

[rdblue]

Referring to "policy" sounds complicated, and listing the "snapshot affected" makes this less clear because snapshot A is affected by multiple policies.

I think I'd drop the affected snapshots column and call policy something like "scope": table scope, branch/main scope, etc.

I like the sections for how this progresses. I think that makes it more clear.

[rdblue]

How do you set "forever"? I think this would make sense if max-ref-age-ms is unset.

[rdblue]

It seems strange to me that H would expire since it is a branch ref. I think that the additional setting solves the problem though.

[rdblue]

I think you should mention that we're assuming all of these snapshots were created at or around the same time.

[rdblue]

Min to keep must be at least 1 because it includes the referenced snapshot.

[rdblue]

Is dev pointing to C or E?

[rdblue]

Like the process above, I think it will be more clear when this focuses on what to retain, rather than what to expire. The problem is that focusing on what to expire begs the question about which setting takes priority for removal. Focusing on what to retain avoids that problem because it is clear that a snapshot may be retained for multiple reasons.

Here, I'd say that:

• main is set to keep 3 days of snapshots, so all ancestors of C newer than 3 days should be kept: (C, B, A)
• b1 is set to keep 2 days of snapshots, so all ancestors of E newer than 2 days are kept: (E, D, A)
• dev retains snapshot E (I think)
• b2 aged off, so it doesn't affect retention

[rdblue]

I think it would help if D expired here and the branch had min-snapshots-to-keep set to 1.

[jackye1995]

@rdblue thanks for the detailed review, I have made the following changes:

1. refs is changed to a map type given the fact that all reference names must be unique in the table
2. max-ref-age-ms is added
3. current-branch is removed
4. I removed the snapshot tagging and branching page for now to make this more focused on the spec change, and moved retention policy into the spec. I will continue to edit the page based on your suggestions, but I think it's better to have that published after the UpdateSnapshotReference API is implemented, so we can also provide code snippets for the specific use cases.

[rymurr]

thanks @jackye1995! I have put my thoughts from the google doc here as a review

[rymurr]

As discussed on the google doc I would be in favour of splitting this into two structures. From the google doc:

To me this is coupling the concept of snapshot expiry to the branch/tag definition when we know we probably don't want to long term. Which could make it hard to make changes in the future. Maybe I am thinking about it too hard but to me there are two concepts: the list of branches/tags and the expiration policies. Having these as two separate data structures allows for both to grow independently in my mind: named policies, more complex policies etc as well as more flexibility in how the list of existing references is maintained in the presence of multi-table branching or catalog owned branching etc.

[rdblue]

I'd be fine either way. I think that the advantage here is that it is simple to express. Having a list of expiration policies and applying them selectively to tags or branches, possibly by identifying the policy by ID seems a bit overkill to me. I think you'd probably have some default branch/tag retention, retention for branch versions, and that's it. Even customizing that per branch seems a little speculative to me. I think people will likely just set expiration globally and forget about it.

[jackye1995]

I think I mentioned this a bit in the design doc's reply, we can always add a reference pointer when there is a need to have a separated retention policy construct, so it's fully backwards compatible. For exmaple:

"branchA": {
  "max-ref-age-ms": 10000
},
"branchB": {
  "retention-policy-id": 2
}

But for now having a separated retention policy object feels a bit overkill.

[rymurr]

My concern raised on the google doc was that the act of re-writing metadata.json and doing a full transaction just to add or change a branch/tag feels strange. Now that current-branch is gone I am a bit less concerned. The only time where there is a transaction just to modify the refs field is when a branch or tag is created, is that right?

The optimistic retry means that at transaction commit time things will unlikely conflict or cause issues. I think it just feels counterintuitive to me to do a re-write of the table metadata just to eg change the branch or add a tag. I also have concerns about extensibility in the future. For example extending to multiple tables, or implementing a git log like feature.

One potential solution is to give the catalogs control over the implementation. For example the HMS catalog may store the proposed branch related data structures in the table definition in Hive rather in the metadata.json. This allows individual catalogs more latitude in how they deal with branching, and the ability to evolve that strategy over time. It does have the downside that migrating between catalogs could be more troublesome.

[rdblue]

The only time where there is a transaction just to modify the refs field is when a branch or tag is created, is that right?

Yes.

feels counterintuitive to me to do a re-write of the table metadata just to eg change the branch or add a tag . . . One potential solution is to give the catalogs control over the implementation.

I agree, this is a bit awkward. But it fits with the current model of keeping metadata.json as the source of truth for a table. I think we can evolve catalogs to do fancier things here -- for one, not sending all of the snapshots back to the client. But as far as the spec is concerned, how would we express that?

[jackye1995]

Maybe one potential middle ground we can settle here is to first add the tagging feature without branching. That is a feature that many users would like to have for use cases like time-based snapshot expiration, and it makes sense to have that as a part of the Iceberg spec. For the branching feature, it was added to the design more for completeness, but so far (at least to me) there is no feature request for that yet. We can add it when the feature request comes and then discuss the best way to go. What do you think? @rdblue @rymurr

[rdblue]

I don't think we should modify the proposal here. We know that lots of snapshots can be a problem, but that's how Iceberg manages snapshots. I don't think we need to hold off on building a feature like this for fairly rare cases. We can add branches and handle the metadata issues elsewhere.

One solution I've been thinking about is allowing table implementations to request a specific time range or updates since some time in the REST catalog. That could handle this problem nicely.

[jackye1995]

makes sense. In that case do you have any additional concern related to the spec change?

[rymurr]

I still feel uncomfortable about a transaction to add a tag but I don't see any easy way out of it. I think it would be good to have a discussion about metadata.json as the source of truth (for everything) in the longer term as I think that is becoming less feasible. My comment on the call today about notification settings living in metadata is related.

I guess my only question is if we agree that its awkward and we agree that catalogs have more of a role to play in teh future then how do we move on from this proposal? Its hard to back this out of the spec or evolve it once its in. I know this is a hard question to reason about and I don't want to hold this useful feature up. But it would be good to at least think about it given the above discussion.

[jackye1995]

Yes I think the work in TableMetadata builder and REST catalog is to try have an alternative way of dealing with table metadata updates from as a diff rather than rewriting the entire table metadata file. And that could open the door for potentially some other implementations of the TableMetadata spec backed by services and do not require full metadata rewrite. That is a development I would be happy to see, and with that we will be able to have fast commits to the metadata for small changes like tagging.

[rdblue]

Yeah, I think we agree that it is awkward. But it's a separate point. We already allow branching to some degree, or at least a collection of snapshots that isn't necessarily linear. Being able to label some of those snapshots and change retention for them doesn't really alter the problem.

@jackye1995 is right that we're getting to a point where we can possibly change this in the future. Moving to a change-based API is one step toward it, in addition to the use case of being able to more easily migrate library versions.

[rdblue]

Should this be something like unset (forever) or Long.MAX_VALUE (forever)?

[jackye1995]

Because this will be used as the default if the value is not set in ref, I am making it Long.MAX_VALUE to avoid an additional null check.

[hameizi]

@rdblue @jackye1995 Can this PR be merged? And then we can continue the following work.

[rdblue]

I would flip the description to be about the reference, not about the snapshot ID. I'd also add context for tags and branches to make it more understandable: "A reference's snapshot ID. The tagged snapshot or latest snapshot of a branch."

[jackye1995]

updated

[rdblue]

Typo: "defaults to" rather than "default to"

[jackye1995]

updated

[rdblue]

You could make this a bit smaller by getting rid of extra words in the second sentence. Also, I think this should state that it includes the latest snapshot.

For branch type only, a positive number for the max age of snapshots to keep when expiring, including the latest snapshot. Defaults to table property history.expire.max-snapshot-age-ms.

[jackye1995]

updated

[rdblue]

I just noticed that only v2 is here. What about v1? I think this should be v1 / v2

[jackye1995]

makes sense, updated

[rdblue]

I left some minor comments, but +1 overall. I think this is ready to go in.

[jackye1995]

Thanks Ryan for the review! I have updated everything based on your comments. Please let me know if you would like a final pass, otherwise I will merge this by the end of the day.