szehon-ho commented on code in PR #16025:
URL: https://github.com/apache/iceberg/pull/16025#discussion_r4252494754
##########
format/spec.md:
##########
@@ -61,6 +61,8 @@ The full set of changes are listed in [Appendix
E](#version-3).
Version 4 of the Iceberg spec restructures metadata for improved performance
and new capabilities:
+* Support for an [adaptive metadata tree](#manifests), enabling efficient
small commits, column updates, and columnar statistics representation
+* Data files and deletion vectors are stored in a combined entry
representation, removing the need for two phase planning
Review Comment:
optional: can clarify entry here:
'are represented in a single manifest entry'
##########
format/spec.md:
##########
@@ -546,7 +551,7 @@ Note that:
### Partitioning
-Data files are stored in manifests with a tuple of partition values that are
used in scans to filter out files that cannot contain records that match the
scan’s filter predicate. Partition values for a data file must be the same for
all records stored in the data file. (Manifests store data files from any
partition, as long as the partition spec is the same for the data files.)
+Data files are stored in manifests with a tuple of partition values that are
used in scans to filter out files that cannot contain records that match the
scan’s filter predicate. Partition values for a data file must be the same for
all records stored in the data file. Manifests store data files from any
partition. v4 manifests may store partitions from any spec, but manifests in v3
and earlier store files for a single spec. A manifest is considered partitioned
by a spec if every entry in the manifest is partitioned by that spec.
Review Comment:
if following @stevenzwu 's other comments, v4+ here as well?
##########
format/spec.md:
##########
@@ -676,15 +693,26 @@ 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"
|
+=== "v4"
+ | Requirement | Key | Value |
+ |-------------|---------------------|-------|
+ | _required_ | `schema-id` | ID of the schema used to write the
manifest as a string |
+ | _required_ | `format-version` | Table format version number of the
manifest as a string |
+
#### 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.
-#### Manifest Entry Fields
+#### Manifest Schema
+
+In v4, manifests store `tracked_file` records that describe data or manifest
files and contain `tracking` metadata, like `status`. The relationship between
a file and its tracking metadata is inverted in v1-v3 manifests, which store
`manifest_entry` records that contain tracking information and a `data_file`
struct. The v4 manifest schema is not compatible with the v1-v3 manifest schema.
Review Comment:
v4+?
##########
format/spec.md:
##########
@@ -476,7 +481,7 @@ A data file with only new rows for the table may omit the
`_last_updated_sequenc
On read, if `_last_updated_sequence_number` is `null` it is assigned the
`sequence_number` of the data file's manifest entry. The data sequence number
of a data file is documented in [Sequence Number
Inheritance](#sequence-number-inheritance).
-When `null`, a row's `_row_id` field is assigned to the `first_row_id` from
its containing data file plus the row position in that data file (`_pos`). A
data file's `first_row_id` field is assigned using inheritance and is
documented in [First Row ID Inheritance](#first-row-id-inheritance). A
manifest's `first_row_id` is assigned when writing the manifest list for a
snapshot and is documented in [First Row ID
Assignment](#first-row-id-assignment). A snapshot's `first-row-id` is set to
the table's `next-row-id` and is documented in [Snapshot Row
IDs](#snapshot-row-ids).
+When `null`, a row's `_row_id` field is assigned to the `first_row_id` from
its containing data file plus the row position in that data file (`_pos`). A
data file's `first_row_id` field is assigned using inheritance and is
documented in [First Row ID Inheritance](#first-row-id-inheritance). A
manifest's `first_row_id` is assigned when writing the snapshot root file and
is documented in [First Row ID Assignment](#first-row-id-assignment). In v4, a
data file in the root manifest is assigned a `first_row_id` in the same way. A
snapshot's `first-row-id` is set to the table's `next-row-id` and is documented
in [Snapshot Row IDs](#snapshot-row-ids).
Review Comment:
The sentence got long. how about put the ref at the end?
```
When writing the snapshot root file, writers assign first_row_id values to
manifests, and in V4 also assign first_row_id to data files tracked directly by
the root manifest. See [First Row ID Assignment]
```
##########
format/spec.md:
##########
@@ -656,15 +661,27 @@ A data or delete file is associated with a sort order by
the sort order's id wit
### Manifests
-A manifest is an immutable Avro file that lists data files or delete files,
along with each file’s partition data tuple, metrics, and tracking information.
One or more manifest files are used to store a [snapshot](#snapshots), which
tracks all of the files in a table at some point in time. Manifests are tracked
by a [manifest list](#manifest-lists) for each table snapshot.
+A table's metadata tree is composed of manifests. A manifest is an immutable
file that tracks a subset of a table metadata for a given
[snapshot](#snapshots). Leaf manifests are the lowest level of the metadata
tree and track data or delete files, along with each file's partition data,
metrics, and tracking information. The snapshot root file tracks leaf
manifests; in v4 and later the root is also a manifest and can also track data
files.
Review Comment:
Nit: leaf is a bit confusing here. Its not clear that a leaf is a root or
not in this paragraph. How about this small re-arrange that separate the
concepts cleanly and defines each?
```
Leaf manifests are non-root manifests that track data or delete files, along
with each file’s partition data, metrics, and tracking information. The
snapshot root file references any leaf manifests. In V4 and later, the root is
also a manifest and may track data files directly, including snapshots with no
leaf manifests.
```
##########
format/spec.md:
##########
@@ -1130,6 +1276,14 @@ When expiring snapshots, retention policies in table and
snapshot references are
2. 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.
+#### Deleting Files
+
+A file may be deleted from the file system when the snapshot in which it was
deleted is garbage collected, assuming that older snapshots have also been
garbage collected [1].
+
+Notes:
+
+1. Technically, data files can be deleted when the last snapshot that contains
the file as "live" data is garbage collected. But this is harder to detect and
requires finding the diff of multiple snapshots. It is easier to track what
files are deleted in a snapshot and delete them when that snapshot expires. It
is not recommended to add a deleted file back to a table. Adding a deleted file
can lead to edge cases where incremental deletes can break table snapshots.
Review Comment:
i guess it's moved, but this is a bit annoying to read as they are two
points. Can we put (2) while we are at it (the part about not re-adding the
file back)
##########
format/spec.md:
##########
@@ -144,8 +146,11 @@ Version 4 of the Iceberg spec adds support for relative
locations in metadata, e
* **Schema** -- Names and types of fields in a table.
* **Partition spec** -- A definition of how partition values are derived from
data fields.
* **Snapshot** -- The state of a table at some point in time, including the
set of all data files.
-* **Manifest list** -- A file that lists manifest files; one per snapshot.
-* **Manifest** -- A file that lists data or delete files; a subset of a
snapshot.
+* **Snapshot root file** -- The per-snapshot file that tracks a snapshot's
manifests; a manifest list (v1-v3) or a root manifest (v4).
+* **Manifest list** -- (v1-v3 only) A file that lists manifest files; one per
snapshot.
+* **Root manifest** -- (v4+) A manifest that can reference leaf manifests,
data files, or v1-v3 manifests; one per snapshot.
Review Comment:
agree, this is a bit confusing as is. how about just remove leaf manifest
and the v1-v3 manifest. They seems equal to the data/delete manifest , which
themselves have definition across versions.
```A manifest that can reference data files, data manifests, or delete
manifests; one per snapshot.```
--
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]
