dramaticlly commented on code in PR #17198:
URL: https://github.com/apache/iceberg/pull/17198#discussion_r3634243956
##########
format/spec.md:
##########
@@ -674,11 +674,13 @@ A manifest file must store the partition spec and other
metadata as properties i
| _optional_ | _required_ | `format-version` | Table format version
number of the manifest as a string
|
| | _required_ | `content` | Type of content files
tracked by the manifest: "data" or "deletes"
|
-The schema of a manifest file is defined by the `manifest_entry` struct,
described in the following section.
+#### Content file uniqueness
+
+Within a snapshot, each content file may be referenced by at most one manifest
entry across all manifests. A snapshot in which more than one entry references
the same content file has undefined behavior. Writers are not required to
validate uniqueness at commit time.
Review Comment:
Nit: the combination of "may" and "at most one" reads to me like it's really
expressing a "must". Curious, Is the phrasing intentional because we want to
avoid the expensive writer enforcement?
On structure side, I read the first sentence as stating the uniqueness
invariant, and the second as its consequence. The first half of the second
sentence ("more than one entry references the same content file") seems to
restate the invariant before getting to the consequence, so it reads a little
repetitive to me. Do you think something like this flow better?
> Within a snapshot, each content file must be referenced by at most one
manifest entry across all manifests; otherwise, the snapshot has undefined
behavior.
##########
format/spec.md:
##########
@@ -1062,7 +1064,7 @@ Scan predicates are also used to filter data and delete
files using column bound
Data files that match the query filter must be read by the scan.
-Note that for any snapshot, all file paths marked with "ADDED" or "EXISTING"
may appear at most once across all manifest files in the snapshot. If a file
path appears more than once, the results of the scan are undefined. Reader
implementations may raise an error in this case, but are not required to do so.
+Manifest entries that reference the same content file violate [content file
uniqueness](#content-file-uniqueness). When duplicates are present, table
behavior (including scan results) is undefined. Reader implementations may
raise an error but are not required to do so.
Review Comment:
I think getting rid of "all file paths " language also align better with DV
in delete manifests, which is uniquely identified by the file_path,
content_offset, content_size_in_bytes instead of location in data file and
position delete.
--
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]
