Repository navigation
Spec: Fix content file uniqueness with REPLACED and MODIFIED entries - #18791
gaborkaszab wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
Specification
| #### Content file uniqueness | ||
|
|
||
| Within a snapshot, each content file must be referenced by at most one live manifest entry across all manifests; otherwise, the snapshot has undefined behavior. Writers should not produce multiple manifest entries for the same content file in a snapshot (for example, both ADDED and DELETED entries for the same file). Writers are not required to validate uniqueness at commit time. | ||
| Within a snapshot, each content file must be referenced by at most one live manifest entry across all manifests; otherwise, the snapshot has undefined behavior. Writers should not produce multiple manifest entries for the same content file in a snapshot (for example, both ADDED and DELETED entries for the same file), except for the REPLACED and MODIFIED pair produced when a file is modified. Writers are not required to validate uniqueness at commit time. |
There was a problem hiding this comment.
Not sure we need the exception? The first sentence already establishes we're talking about live manifest entries, and REPLACED is not live?
There was a problem hiding this comment.
Ah okay but the second sentence says "Writers should not produce multiple manifest entries..." which can read confusing for replaced/modified
There was a problem hiding this comment.
Yes, the second sentence doesn't allow to add the REPLACED+MODIFIED pair in one snapshot
There was a problem hiding this comment.
Ok how about
Within a snapshot, each content file must be referenced by at most one [live](#manifest-schema) manifest entry across all manifests; otherwise, the snapshot has undefined behavior. Writers should not produce a DELETED entry for a content file that also has a live entry in the same snapshot. Writers are not required to validate uniqueness at commit time.
this does a couple things, link what live means and also instead of wording it in terms of the exception for replaced + modified, we phrase it in terms of the specific case we were trying to avoid on the original change that added this which is preventing DELETED + any live status (but it's a should instead of a must because technically writers, such as the java implementation do that today)? and I think this wording implciitly allows for the REPLACED + MODIFIED case without having to restate that requirement.
There was a problem hiding this comment.
I'm fine with this wording too. I would like us to avoid enumerating states if we can help it.
There was a problem hiding this comment.
Thanks for the suggestion, @amogh-jahagirdar !
Just trying to think it through from all angles before doing the change:
With saying what you suggest aren't we in the situation that some status combinations aren't covered by the wording. E.g. This uniqueness paragraph says DELETED is not allowed together with live entries, but doesn't say anything about DELETED + REPLACED combination. Both are non-live so none of the sentences protect against that. Rules for REPLACED might be covered somewhere else in the spec, but the uniqueness section isn't self contained then.
With saying what I suggested, that it's not allowed to have multiple entries for the same file except for the REPLACED + MODIFIED combo we kind of covered all the combinations, and maybe safe for potential future status changes too.
WDYT @amogh-jahagirdar @RussellSpitzer ?
No description provided.