Core: Add file seq number to ManifestEntry by aokolnychyi · Pull Request #6002 · apache/iceberg

aokolnychyi · 2022-10-17T15:31:44Z

This PR adds file_sequence_number to ManifestEntry to keep track of the sequence number at which a file was added.

aokolnychyi · 2022-10-17T15:32:53Z

   *
   * @param deletedFile a file
-   * @deprecated since 1.0.0, will be removed in 1.1.0; use {@link #delete(ContentFile, long)}.
+   * @deprecated since 1.1.0, will be removed in 1.2.0; use {@link #delete(ContentFile, long,


This did not make it to 1.0, just fixing the comment.

aokolnychyi · 2022-10-17T15:33:15Z

   * @param dataSequenceNumber a data sequence number of the file (assigned when the file was added)
+   * @param fileSequenceNumber a file sequence number (assigned when the file was added)
   */
-  public void delete(F deletedFile, long dataSequenceNumber) {


This was added recently and hasn't been released yet.

aokolnychyi · 2022-10-17T15:33:50Z

+   * @param fileSequenceNumber a file sequence number (assigned when the file was added)
+   */
+  public void existing(
+      F existingFile, long fileSnapshotId, long dataSequenceNumber, Long fileSequenceNumber) {


Note fileSequenceNumber is Long because the value may be null in old v2 manifests.

aokolnychyi · 2022-10-17T15:35:03Z

+    validateManifest(
+        committedManifest,
+        seqs(appendSequenceNumber, appendSequenceNumber),
+        seqs(null, null),


This test mimics a rewrite of a v2 manifest that did not have file sequence numbers.

aokolnychyi · 2022-10-17T15:38:24Z

        this.sequenceNumber = null;
        return;
      case 3:
+        this.fileSequenceNumber = (Long) v;


Let me see if it safe to add it here. I think we use positions in RewriteManifestsSparkAction.

Okay, it seems safe as we use a projection there. That being said, I will need to adapt it in a follow-up to persist the file sequence number as well.

Should we do it in this PR rather than in a follow up? That seems like an important part and we don't want to release this without it.

I agree that part must be done before releasing fileSequenceNumber. However, I'd do it in a separate PR immediately after this one as fileSequenceNumber is optional and the current logic does not cause correctness issues. It would be just easier to separate these two.

chenjunjiedada · 2022-10-18T09:40:19Z

+   * reading a v2 manifest that did not persist the file sequence number for manifest entries with
+   * status EXISTING or DELETED (older Iceberg versions).
+   */
+  Long fileSequenceNumber();


Do we have any sequence number usage case that cannot be handled by only using fileSequenceNumber? I 'm wondering can we replace dataSequenceNumber with fileSequenceNumber eventually.

I think we need both as they serve different purposes. The data sequence number will still be used to decide whether a particular delete should apply. The file sequence number, on the other hand, will always tell us the sequence number of the snapshot that added this file. These values can be different during compaction as we may add a file with an old data sequence number.

Oh, I see. Deletes should not apply to files that have larger dataSequenceNumber but smaller fileSequenceNumber. I guess when compacting data files with deletes may lead to adding existing files with old fileSequenceNumber, compacting v1 table should not have that case.

So we should add dataSequenceNumber to ContentFile since it will be used for planning, right?

We already use dataSequenceNumber / sequenceNumber in scan planning. We just renamed how it was referenced in core recently. If we choose to expose these on ContentFile that's fine, but something we should do in a separate PR.

Correct, these sequence numbers would serve different purposes and may be added to ContentFile later.

IIUC, assuming df1 and df2 have data sequence number 1, once they are compacted to one data file df3. df3's data sequence number will be 1(the old one), and file sequence number is gong to be larger than 1, like 2 or 3. Is that correct?

How do we assign the data data sequence number, if df1 and df2 has different data sequence numbers?

df1 data seq no.: 1 df2 data seq no.: 2 compact df1 and df2 to df3 df3 data seq no.?

@flyrain, this is something controlled by the following option in the compaction action.

/** * If the compaction should use the sequence number of the snapshot at compaction start time for * new data files, instead of using the sequence number of the newly produced snapshot. * * <p>This avoids commit conflicts with updates that add newer equality deletes at a higher * sequence number. * * <p>Defaults to true. */ String USE_STARTING_SEQUENCE_NUMBER = "use-starting-sequence-number";

By default, df3 will use the sequence number of the snapshot at which the compaction started.

chenjunjiedada

LGTM, just one question.

rdblue · 2022-10-23T20:09:33Z

+            "File sequence number should default to 0", (Long) 0L, entry.fileSequenceNumber());
+        V2Assert.assertEquals(
+            "File sequence number should match", fileSeqs.next(), entry.fileSequenceNumber());
+      }


Nit: would be nice to have empty lines before and after the new block.

I agree but it is not how this entire block is formatted. None of the other if statements are separated by empty lines. I decided to follow the existing formatting for consistency even though I'd surround each independent block by empty lines.

rdblue · 2022-10-23T20:18:07Z

   */
-  public void delete(F deletedFile, long dataSequenceNumber) {
-    addEntry(reused.wrapDelete(snapshotId, dataSequenceNumber, deletedFile));
+  public void delete(F deletedFile, long dataSequenceNumber, Long fileSequenceNumber) {


It doesn't look like any of these delete methods are used, besides delete(ManifestEntry). Do we need to introduce this new method or can we update the deprecation for delete(ContentFile) to point people to delete(ManifestEntry)?

The problem is that delete(ManifestEntry) is package-private just like ManifestEntry itself. This, on the other hand, is a public method.

rdblue

Looks correct to me. I had a few comments but merge when you're ready. The main question is whether to fix data file rewrites at the same time, but the big issue is making sure it gets done in the same release, not necessarily in this PR.

flyrain · 2022-10-26T20:01:38Z

-Iceberg v2 adds a sequence number to the entry and makes the snapshot id optional. Both fields, `sequence_number` and `snapshot_id`, are inherited from manifest metadata when `null`. That is, if the field is `null` for an entry, then the entry must inherit its value from the manifest file's metadata, stored in the manifest list [2].
-The `sequence_number` field represents the data sequence number and must never change after a file is added to the dataset. 
+Iceberg v2 adds data and file sequence numbers to the entry and makes the snapshot ID optional. Values for these fields are inherited from manifest metadata when `null`. That is, if the field is `null` for an entry, then the entry must inherit its value from the manifest file's metadata, stored in the manifest list.
+The `sequence_number` field represents the data sequence number and must never change after a file is added to the dataset. The `file_sequence_number` field represents the sequence number of the snapshot that added the file and must also remain unchanged upon assigning at commit.


I feel a bit hard to understand the difference between them. Can we add something like?

The `sequence_number` will use the one from original files in the case of compaction, but the `file_sequence_number` will alway be one from the snapshot added the file. `file_sequence_number` of a file may be larger or the same as its `sequence_number`.

The sequence number in case of compaction would be the compaction snapshot seq number, not necessarily the original sequence number. However, I agree a clarification is needed. I adapted this paragraph to include more details.

aokolnychyi · 2022-10-27T04:33:01Z

Thanks, @chenjunjiedada @rdblue @flyrain! I'll follow up with a PR to take the new field into account while rewriting manifests in Spark.

…e#6002)

aokolnychyi commented Oct 17, 2022

View reviewed changes

github-actions Bot added the core label Oct 17, 2022

aokolnychyi commented Oct 17, 2022

View reviewed changes

aokolnychyi force-pushed the file-sequence-number branch from 9edc478 to 9ea3af8 Compare October 17, 2022 18:34

github-actions Bot added the spark label Oct 17, 2022

aokolnychyi requested review from chenjunjiedada, flyrain, jackye1995, rdblue and szehon-ho October 17, 2022 18:35

chenjunjiedada reviewed Oct 18, 2022

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/InheritableMetadataFactory.java

chenjunjiedada reviewed Oct 18, 2022

View reviewed changes

chenjunjiedada approved these changes Oct 18, 2022

View reviewed changes

aokolnychyi force-pushed the file-sequence-number branch from 9ea3af8 to d9dcd08 Compare October 18, 2022 14:55

rdblue reviewed Oct 23, 2022

View reviewed changes

Comment thread core/src/test/java/org/apache/iceberg/TestFastAppend.java Outdated

rdblue reviewed Oct 23, 2022

View reviewed changes

Comment thread format/spec.md Outdated

rdblue approved these changes Oct 23, 2022

View reviewed changes

flyrain reviewed Oct 26, 2022

View reviewed changes

flyrain approved these changes Oct 26, 2022

View reviewed changes

Core: Add file seq number to ManifestEntry

928b006

aokolnychyi force-pushed the file-sequence-number branch from d9dcd08 to 928b006 Compare October 26, 2022 20:59

aokolnychyi merged commit 8bff303 into apache:master Oct 27, 2022

aokolnychyi mentioned this pull request Nov 9, 2022

Respect fileSequenceNumber in RewriteManifestsSparkAction #6162

Closed

ajantha-bhat mentioned this pull request Dec 9, 2022

First version of the changelog apache/iceberg-docs#185

Merged

zhongyujiang pushed a commit to zhongyujiang/iceberg that referenced this pull request Apr 16, 2025

[Cherry-Pick] Spec, Core: Add file seq number to ManifestEntry (apach…

8e3cb51

…e#6002)

Uh oh!

Conversation

aokolnychyi commented Oct 17, 2022

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!

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!

chenjunjiedada Oct 18, 2022 • 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!

chenjunjiedada Oct 18, 2022 • 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!

flyrain Oct 26, 2022 • 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!

aokolnychyi Oct 26, 2022 • 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!

chenjunjiedada 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rdblue left a comment

Choose a reason for hiding this comment

Uh oh!

flyrain Oct 26, 2022 • 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!

aokolnychyi commented Oct 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chenjunjiedada Oct 18, 2022 •

edited

Loading

chenjunjiedada Oct 18, 2022 •

edited

Loading

flyrain Oct 26, 2022 •

edited

Loading

aokolnychyi Oct 26, 2022 •

edited

Loading

flyrain Oct 26, 2022 •

edited

Loading