Add v2 manifest lists #907

rdblue · 2020-04-09T19:18:08Z

This extends #903 with a v2 manifest list. The v2 schema is not final and may change. This is mainly to implement a separate write path and a framework for compatibility tests.

Specific changes:

Split ManifestFile schema into v1 and v2 (with sequence numbers)
Update GenericManifestFile to v2 schema
Update v1 manifest list writer to use the v1 schema
Add a v2 manifest list writer
Add tests for v1 and v2 manifest list formats

rdblue · 2020-04-09T21:28:57Z

core/src/test/java/org/apache/iceberg/TestGenericManifestFile.java

-  public TemporaryFolder temp = new TemporaryFolder();
-
-  @Test
-  public void testManifestsWithoutRowStats() throws IOException {


Moved into TestManifestFileVersions.

chenjunjiedada · 2020-04-10T09:32:44Z

+1

rdblue · 2020-04-10T19:40:45Z

Instead of making spec changes in each v2 commit I'm using a v2-spec PR, #912. That way we don't trigger CI runs in PRs that are waiting to be merged for updates to the spec.

api/src/main/java/org/apache/iceberg/ManifestFile.java

* Update GenericManifestFile to v2 schema * Update v1 manifest list writer to use the v1 schema * Add a v2 manifest list writer * Add tests for v1 and v2 manifest list formats

aokolnychyi · 2020-04-14T20:44:13Z

api/src/main/java/org/apache/iceberg/ManifestFile.java

-      optional(512, "added_rows_count", Types.LongType.get()),
-      optional(513, "existing_rows_count", Types.LongType.get()),
-      optional(514, "deleted_rows_count", Types.LongType.get()));
+      PATH, LENGTH, SPEC_ID,


I remember we had issues with reordering fields in ManifestFile as GenericAvroWriter was using ordinal positions instead of field ids. Did we solve that?

I see this is handled in V1Metadata.

Yeah, the IndexedRecord field order needs to match the schema order. This is why I added a test for this as well.

aokolnychyi · 2020-04-14T20:54:34Z

core/src/main/java/org/apache/iceberg/GenericManifestFile.java

    this.fromProjectionPos = null;
  }

-  public GenericManifestFile(String path, long length, int specId, Long snapshotId,


Even though it was public, I don't think anyone is using it. Seems OK to change.

Yeah, this should be fine. Classes in core are only semi-public and not part of the API. That module has stronger guarantees.

core/src/main/java/org/apache/iceberg/ManifestListWriter.java

aokolnychyi · 2020-04-14T21:31:56Z

core/src/main/java/org/apache/iceberg/GenericManifestFile.java

  private Integer existingFilesCount = null;
-  private Long existingRowsCount = null;
  private Integer deletedFilesCount = null;
+  private Long addedRowsCount = null;


Looks like we are trying to match the new ordering of fields in ManifestFile. Earlier, we co-located ...FilesCount with ...RowsCount to match the ordering of methods in ManifestFile and args in constructors. Is this change intentional?

Yes. I think it's likely that these are going to be null in some cases, like manifests that contain equality delete files. Instead of mixing null, non-null, null, non-null, etc. I think it's better to keep the probably-null columns colocated for compression.

aokolnychyi · 2020-04-14T22:21:51Z

core/src/main/java/org/apache/iceberg/ManifestListWriter.java

  static class V1Writer extends ManifestListWriter {
-    private V1Writer(OutputFile snapshotFile, long snapshotId, Long parentSnapshotId) {
-      super(snapshotFile, snapshotId, parentSnapshotId);
+    private final V1Metadata.IndexedManifestFile wrapper = new V1Metadata.IndexedManifestFile();


Seems like this is the place where we can pass a snapshot id similar to how we pass a sequence number to V2 and get rid of the logic for inheriting metadata for ManifestEntry via setSnapshotId and iterating through manifests during commit.

Do I get it correctly, @rdblue?

Yes. I'm going to rework inheritance for snapshot ID in a separate commit.

aokolnychyi

+1

aokolnychyi · 2020-04-14T23:41:11Z

I've merged this. Thanks, @rdblue!

rdblue commented Apr 9, 2020

View reviewed changes

rdblue commented Apr 10, 2020

View reviewed changes

api/src/main/java/org/apache/iceberg/ManifestFile.java Show resolved Hide resolved

rdblue force-pushed the v2-manifest-lists branch from 456f2ca to 220c6a5 Compare April 10, 2020 23:30

rdblue mentioned this pull request Apr 11, 2020

Add v2 manifests #913

Merged

rdblue added 3 commits April 11, 2020 13:37

Add v2 manifest lists.

3b2fbd0

* Update GenericManifestFile to v2 schema * Update v1 manifest list writer to use the v1 schema * Add a v2 manifest list writer * Add tests for v1 and v2 manifest list formats

Write ManifestFile sequence number in v2 writer.

ba51dda

Make static metadata variables final.

f564456

rdblue force-pushed the v2-manifest-lists branch from 00cb37d to f564456 Compare April 11, 2020 20:38

rdblue requested review from aokolnychyi and danielcweeks April 14, 2020 18:56

aokolnychyi reviewed Apr 14, 2020

View reviewed changes

Remove trailing newlines.

8b1e316

rdblue requested a review from aokolnychyi April 14, 2020 22:37

aokolnychyi approved these changes Apr 14, 2020

View reviewed changes

aokolnychyi merged commit 487fc1c into apache:master Apr 14, 2020

waterlx added a commit to waterlx/incubator-iceberg that referenced this pull request Apr 21, 2020

Quick fix to comply with Add v2 manifest lists (apache#907)

db50faa

Fokko pushed a commit to Fokko/iceberg that referenced this pull request Apr 21, 2020

Add v2 manifest lists (apache#907)

a0f1a1b

waterlx added a commit to waterlx/incubator-iceberg that referenced this pull request Apr 26, 2020

Quick fix to comply with Add v2 manifest lists (apache#907)

15d93f3

waterlx added a commit to waterlx/incubator-iceberg that referenced this pull request Apr 27, 2020

Quick fix to comply with Add v2 manifest lists (apache#907)

5757c08

waterlx added a commit to waterlx/incubator-iceberg that referenced this pull request May 6, 2020

Quick fix to comply with Add v2 manifest lists (apache#907)

9fe722f

rdblue added this to the Row-level Delete milestone May 8, 2020

waterlx added a commit to waterlx/incubator-iceberg that referenced this pull request May 9, 2020

Quick fix to comply with Add v2 manifest lists (apache#907)

0187f7e

waterlx added a commit to waterlx/incubator-iceberg that referenced this pull request May 27, 2020

Quick fix to comply with Add v2 manifest lists (apache#907)

90dbdd3

waterlx added a commit to waterlx/incubator-iceberg that referenced this pull request May 28, 2020

Quick fix to comply with Add v2 manifest lists (apache#907)

c3e692c

waterlx added a commit to waterlx/incubator-iceberg that referenced this pull request Jun 3, 2020

Quick fix to comply with Add v2 manifest lists (apache#907)

a98994b

yshcz mentioned this pull request Dec 24, 2025

Spec: Add implementation note for determining manifest list format version #14926

Open

3 tasks

Add v2 manifest lists #907

Add v2 manifest lists #907

Uh oh!

Conversation

rdblue commented Apr 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chenjunjiedada commented Apr 10, 2020

Uh oh!

rdblue commented Apr 10, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aokolnychyi Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aokolnychyi left a comment

Choose a reason for hiding this comment

Uh oh!

aokolnychyi commented Apr 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aokolnychyi Apr 14, 2020 •

edited

Loading