Core: Add content field to ManifestTable Schema. #2510

coolderli · 2021-04-25T12:19:48Z

We need to add the content field to the schema of AllManifestsTable, so that we can get the type of manifest for expiration processing.

coolderli · 2021-04-26T02:34:26Z

@rdblue @openinx could you help me review it? thanks

openinx · 2021-04-26T02:50:34Z

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

-          Types.NestedField.required(11, "contains_nan", Types.BooleanType.get()),
-          Types.NestedField.optional(12, "lower_bound", Types.StringType.get()),
-          Types.NestedField.optional(13, "upper_bound", Types.StringType.get())
+      Types.NestedField.optional(4, "content", Types.IntegerType.get()),


The correct way to add a new field in this manifest table is : add the content field with an auto-increment field id (which should be 14 in this case) at the tail of those nested field list. We cannot change the existing field ids in this schema because them are mapping to the underlying rows in generated avro files.

OK, I will fix it. Thanks.

Prefer to add it as the last entry so that ID of other fields are not modified for backwards compatibility.

openinx

@coolderli I'm still not quite understand why you want to add the manifest content id in those two table schema. Would you mind to provide more background about this changes ? What's the specific behavior that we could not accomplish in the current table schema ?

coolderli · 2021-04-26T03:39:31Z

@openinx I want to support the expiring snapshot of the V2 table
After we rewrite the data file and the delete file in #2303, we can delete the corresponding data files and delete files through manifest when expiring the snapshot.

openinx · 2021-04-26T03:58:52Z

So why need to add the extra nested field in table schema ? I still not get the point.

coolderli · 2021-04-26T12:17:44Z

Sorry, I didn't make it clear. This patch is actually a part of #2518.

jackye1995 · 2021-04-27T04:03:54Z

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

-          Types.NestedField.required(11, "contains_nan", Types.BooleanType.get()),
-          Types.NestedField.optional(12, "lower_bound", Types.StringType.get()),
-          Types.NestedField.optional(13, "upper_bound", Types.StringType.get())
+      Types.NestedField.optional(4, "content", Types.IntegerType.get()),


Prefer to add it as the last entry so that ID of other fields are not modified for backwards compatibility.

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

jackye1995

Please add some tests if possible, thank you!

jackye1995 · 2021-04-27T06:36:17Z

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

-  );
+      ))),
+      Types.NestedField.optional(14, "content", Types.StringType.get())
+      );


nit: should not change space for unmodified lines.

jackye1995 · 2021-04-27T06:36:27Z

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

-  );
+      ))),
+      Types.NestedField.required(14, "content", Types.StringType.get())
+      );


nit: should not change space for unmodified lines.

jackye1995 · 2021-04-27T06:36:38Z

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

-    );
+        partitionSummariesToRows(spec, manifest.partitions()),
+        manifest.content().name()
+        );


nit: should not change space for unmodified lines.

jackye1995 · 2021-04-27T17:34:02Z

Sorry for the back and forth, now I think about it, the files table has this in schema:

optional(134, "content", IntegerType.get(), "Contents of the file: 0=data, 1=position deletes, 2=equality deletes");

So it would make more sense to use int also for manifest, but we should add a doc similar to the files table like:

optional(14, "content", IntegerType.get(), "Contents of the manifest: 0=data, 1=deletes");

so that people know what the values mean.

Apart from that, I don't have any further comments, thanks for the work.

coolderli · 2021-04-28T03:10:48Z

@jackye1995 Thank you for reviewing the code.

rdblue · 2021-05-20T00:26:55Z

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

  }
+
+  @Test
+  public void testReadDataManifestTable() throws IOException {


I don't think that these tests should be in this class. This suite tests the manifest reader, not the manifest metadata tables. Why not put these tests in TestIcebergSourceTablesBase?

I think this test has covered the use cases.

github-actions · 2024-07-17T00:13:11Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

github-actions · 2024-07-24T00:13:52Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

github-actions bot added core spark labels Apr 25, 2021

coolderli changed the title ~~Core: Add content to ManifestTable Schema.~~ Core: Add content field to ManifestTable Schema. Apr 25, 2021

openinx reviewed Apr 26, 2021

View reviewed changes

jackye1995 reviewed Apr 27, 2021

View reviewed changes

coolderli marked this pull request as draft April 27, 2021 07:03

coolderli marked this pull request as ready for review April 27, 2021 09:03

coolderli force-pushed the add-content branch from f9e4e9d to 79e9c84 Compare April 27, 2021 09:11

coolderli marked this pull request as draft April 28, 2021 02:12

Core: Add content to ManifestTable Schema.

1e261c5

coolderli force-pushed the add-content branch from 7907b58 to 1e261c5 Compare April 28, 2021 03:03

coolderli marked this pull request as ready for review April 28, 2021 03:06

rdblue reviewed May 20, 2021

View reviewed changes

update

54bd7a0

szehon-ho mentioned this pull request May 5, 2022

Spark: Add positional and equality delete file count to ExpireSnapshot results #4629

Merged

github-actions bot added the stale label Jul 17, 2024

github-actions bot closed this Jul 24, 2024

Core: Add content field to ManifestTable Schema. #2510

Core: Add content field to ManifestTable Schema. #2510

Uh oh!

Conversation

coolderli commented Apr 25, 2021

Uh oh!

coolderli commented Apr 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackye1995 Apr 27, 2021 • 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!

openinx left a comment

Choose a reason for hiding this comment

Uh oh!

coolderli commented Apr 26, 2021

Uh oh!

openinx commented Apr 26, 2021

Uh oh!

coolderli commented Apr 26, 2021

Uh oh!

jackye1995 Apr 27, 2021 • 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!

Uh oh!

jackye1995 left a comment

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!

jackye1995 commented Apr 27, 2021

Uh oh!

coolderli commented Apr 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 17, 2024

Uh oh!

github-actions bot commented Jul 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jackye1995 Apr 27, 2021 •

edited

Loading

jackye1995 Apr 27, 2021 •

edited

Loading