Core: Add dataSequenceNumber to ManifestEntry #5913

aokolnychyi · 2022-10-04T16:03:57Z

This PR fixes the behavior of sequence_number in ManifestEntry to always represent the data sequence number.

aokolnychyi · 2022-10-04T16:05:19Z

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

@@ -126,7 +126,7 @@ static Schema schema() {
  /** Returns the sequence number of the commit that added the manifest file. */
  long sequenceNumber();

-  /** Returns the lowest sequence number of any data file in the manifest. */
+  /** Returns the lowest data sequence number of any live file in the manifest. */
  long minSequenceNumber();


I believe this is what was happening before as entries with status DELETED had null as their sequence numbers, which made the writer ignore such entries while computing the min sequence number.

No behavior change, just updating the comment.

aokolnychyi · 2022-10-04T16:06:00Z

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

  @Override
  public Long sequenceNumber() {
-    return sequenceNumber;
+    if (sequenceNumber != null) {


I kept both fields for now as sequenceNumber may be set explicitly during inheritance (DELETEd only).

aokolnychyi · 2022-10-04T16:08:21Z

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

+   *
+   * @deprecated since 1.0.0, will be removed in 1.1.0; use {@link #dataSequenceNumber()} instead.
+   */
+  @Deprecated


I am debating whether we should deprecate this method or simply remove (prior to 1.0). It is core, not API. It is actually pretty dangerous to use this method as it behaves inconsistently.

If needed, we may add a new method to return the sequence number of the manifest entry.

I'd prefer removing it.

aokolnychyi · 2022-10-04T16:10:19Z

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

@@ -368,8 +368,9 @@ private boolean manifestHasDeletedFiles(
          deletePaths.contains(file.path())
              || dropPartitions.contains(file.specId(), file.partition())
              || (isDelete
-                  && entry.sequenceNumber() > 0
-                  && entry.sequenceNumber() < minSequenceNumber);
+                  && entry.isLive()


Attention! I think a delete manifest has files to delete ONLY if the entry status is ADDED or EXISTING. Also, we can't use entry.dataSequenceNumber() otherwise as it may return null for old V2 manifests with DELETED entries.

This looks good to me.

aokolnychyi · 2022-10-04T16:12:05Z

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

-        && (minSequenceNumber == null || entry.sequenceNumber() < minSequenceNumber)) {
-      this.minSequenceNumber = entry.sequenceNumber();
+
+    if (entry.isLive()


Attention! I think there is no behavior change as sequenceNumber() was returning null for DELETEd entries.
See here for details.

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

aokolnychyi · 2022-10-04T16:15:30Z

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

            Preconditions.checkState(
                wrapped.snapshotId() == null || wrapped.snapshotId().equals(commitSnapshotId),
                "Found unassigned sequence number for an entry from snapshot: %s",
                wrapped.snapshotId());
+
+            // inheritance should work only for ADDED entries
+            Preconditions.checkState(


This is new validation. Any cases when it is not safe to validate this? This class is used during writes.

This sounds good to me.

aokolnychyi · 2022-10-04T16:16:01Z

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

@@ -243,7 +243,7 @@ public void testValidateDataFilesExistFromSnapshot() {
    // manifest with FILE_A deleted
    validateManifest(
        snap.dataManifests(table.io()).get(1),
-        seqs(2, 1),


These test changes reflect the new behavior for DELETED entries.

rdblue · 2022-10-06T23:40:16Z

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

+      // in v2 tables, the data sequence number should be inherited iff the entry status is ADDED
+      if (manifestEntry.dataSequenceNumber() == null
+          && (sequenceNumber == 0 || manifestEntry.status() == ManifestEntry.Status.ADDED)) {
+        manifestEntry.setDataSequenceNumber(sequenceNumber);


Looks good to me.

rdblue · 2022-10-06T23:41:35Z

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

    this.sequenceNumber = toCopy.sequenceNumber;
    this.file = toCopy.file().copy(fullCopy);
  }

-  ManifestEntry<F> wrapExisting(Long newSnapshotId, Long newSequenceNumber, F newFile) {
+  ManifestEntry<F> wrapExisting(Long newSnapshotId, Long newDataSequenceNumber, F newFile) {


Nit: it would be fewer changes if we didn't rename newSequenceNumber to newDataSequenceNumber but I can see why you'd want to.

Let me think. I wanted to be explicit in this case as we will also add fileSequenceNumber next.

I decided to go with the rename as we have two variables now and I plan to add fileSequenceNumber next.

@aokolnychyi , What class do you plan to add fileSequenceNumber? DataFile and DeleteFile?

@chenjunjiedada, I'd say we add it to ManifestEntry first (to store as part of manifest entry) and then we can propagate it via ContentFile.

@aokolnychyi, Thanks for the explanation, that will be very helpful.

@chenjunjiedada, I recall you had a PR for propagating seq numbers to ContentFile. I'll reach out once this part is done so that you can rebase your work.

@aokolnychyi , Yeap, actually I have two PRs regarding sequence number, will rebase them when you finish these.

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

rdblue · 2022-10-06T23:57:20Z

Overall, this looks good to me. I think that the removed method should be kept in favor of one that throws UnsupportedOperationException though.

aokolnychyi · 2022-10-09T01:13:35Z

Thanks for reviewing, @rdblue!

github-actions bot added API core labels Oct 4, 2022

aokolnychyi commented Oct 4, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/ManifestWriter.java Show resolved Hide resolved

aokolnychyi commented Oct 4, 2022

View reviewed changes

aokolnychyi requested review from jackye1995, rdblue, szehon-ho, flyrain, danielcweeks, Fokko and nastra October 4, 2022 16:16

Core: Add dataSequenceNumber to ManifestEntry

5043741

aokolnychyi force-pushed the data-seq branch from cae407b to 5043741 Compare October 4, 2022 18:05

rdblue reviewed Oct 6, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/GenericManifestEntry.java Outdated Show resolved Hide resolved

rdblue approved these changes Oct 6, 2022

View reviewed changes

aokolnychyi added 2 commits October 7, 2022 15:59

Review feedback

1e1e16e

Fix javadoc

75c2f05

aokolnychyi merged commit 5e5c6d1 into apache:master Oct 9, 2022

svrajesh21 mentioned this pull request Jan 22, 2025

PR Analysis and Follow-Up: Review of PR #5913 - Data Sequence Number in ManifestEntry #12039

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Add dataSequenceNumber to ManifestEntry #5913

Core: Add dataSequenceNumber to ManifestEntry #5913

aokolnychyi commented Oct 4, 2022

aokolnychyi Oct 4, 2022 •

edited

Loading

aokolnychyi Oct 4, 2022 •

edited

Loading

aokolnychyi Oct 4, 2022 •

edited

Loading

rdblue Oct 6, 2022

aokolnychyi Oct 4, 2022

rdblue Oct 6, 2022

aokolnychyi Oct 4, 2022

aokolnychyi Oct 4, 2022

rdblue Oct 6, 2022

aokolnychyi Oct 4, 2022

rdblue Oct 6, 2022

rdblue Oct 6, 2022

aokolnychyi Oct 7, 2022

aokolnychyi Oct 7, 2022

chenjunjiedada Oct 11, 2022

aokolnychyi Oct 11, 2022

chenjunjiedada Oct 12, 2022 •

edited

Loading

aokolnychyi Oct 13, 2022

chenjunjiedada Oct 14, 2022

rdblue commented Oct 6, 2022

aokolnychyi commented Oct 9, 2022

Core: Add dataSequenceNumber to ManifestEntry #5913

Core: Add dataSequenceNumber to ManifestEntry #5913

Conversation

aokolnychyi commented Oct 4, 2022

aokolnychyi Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

aokolnychyi Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

aokolnychyi Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenjunjiedada Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue commented Oct 6, 2022

aokolnychyi commented Oct 9, 2022

aokolnychyi Oct 4, 2022 •

edited

Loading

aokolnychyi Oct 4, 2022 •

edited

Loading

aokolnychyi Oct 4, 2022 •

edited

Loading

chenjunjiedada Oct 12, 2022 •

edited

Loading