[WIP] Add sequence number for supporting row level delete #588

chenjunjiedada · 2019-10-28T16:30:21Z

This is an initial commit, need some discussion and review.

aokolnychyi · 2019-10-29T15:15:23Z

@chenjunjiedada, thanks for working on this! I'll be interested to review whenever I have a bit of time.

aokolnychyi · 2019-11-08T15:14:16Z

@chenjunjiedada, sorry for the delay. I hope to have more time next week.

rdblue · 2019-11-11T17:56:50Z

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

+   *
+   * @param seqNum the sequential number to assign
+   */
+  void setSeqNum(long seqNum);


This interface should not include a setter method. DataFile is effectively immutable.

rdblue · 2019-11-11T17:58:19Z

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

-        optional(132, "split_offsets", ListType.ofRequired(133, LongType.get()))
-        // NEXT ID TO ASSIGN: 134
+        optional(132, "split_offsets", ListType.ofRequired(133, LongType.get())),
+        optional(134, "sequential_number", LongType.get())


I think the field should be named "sequence_number".

rdblue · 2019-11-11T21:55:34Z

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

  }

-  private ManifestFile writeManifest() throws IOException {
+  private ManifestFile writeManifest(long availSeqNum) throws IOException {


Sequence numbers are not written into manifests during commit because the order of commits is not known until the commit succeeds. Retries require changing the sequence number.

To get around this, sequence numbers are written into the manifest list for each ManifestFile object, and are inherited when reading a manifest.

Make sense.

rdblue · 2019-11-11T21:58:20Z

@chenjunjiedada, I think a good place to start would be to add lastSequenceNumber field to Snapshot that increments each commit, then we can move to the rest of the metadata files from there, starting with ManifestList.

chenjunjiedada · 2019-11-15T15:38:09Z

@rdblue , sorry for the late response. I will update this weekend.

chenjunjiedada · 2019-11-18T06:31:09Z

@rdblue, PR updated. Here I use sequenceNumber instead of lastSequenceNumber since it is referred to the current snapshot and need to be serialized, make sense?

aokolnychyi · 2019-11-25T09:57:19Z

core/src/main/java/org/apache/iceberg/util/JsonUtil.java

+    }
+    JsonNode pNode = node.get(property);
+    Preconditions.checkArgument(pNode != null && !pNode.isNull() && pNode.isNumber() && pNode.canConvertToLong(),
+        "Cannot parse %s from non-string value: %s", property, pNode);


nit: should the error message say "non-string" or "non-numeric"?

aokolnychyi · 2019-11-25T09:58:31Z

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

  }

+  @Override
+  public Long sequenceNumber() {


Hm, does this mean that all existing snapshots written without sequence numbers will have sequenceNumber as 0?

That's an initial implementation that needs some discussion. For example, we can generate sequence numbers from the timestamp for existing snapshots.

I think the timestamp-based approach would work for existing snapshots, but I am not sure how we will propagate this info down to manifests and files. For example, ManifestFile keeps track of the snapshot in which the manifest was added, but that snapshot can be already expired, so we won't be able to fetch its metadata. Maybe, defaulting to 0 won't be an issue, let me think more about this.

@rdblue, any thoughts on keeping backward compatibility?

When we start using sequence numbers, anything without a sequence number should be considered older and should default to 0. That will work as long as there is some time at which we always use sequence numbers.

What wouldn't work is if we have some writers that use sequence numbers and some writers that do not because the ones that don't write sequence numbers would be sequenced before writes that do: if some writer creates seq 1 and an older writer commits later, the older writer's new commit would have seq 0. That new writer would probably also drop all sequence numbers from metadata -- the table sequence number and the one for each ManifestFile stored in the ManifestList.

One more thing to note: the older writer would reset sequence numbers, but metadata written by newer clients could still contain explicit sequence numbers. For example, a new writer that performs a compaction with older data will write the older data's sequence numbers into metadata. If the sequence number gets reset to 0 by an older writer after that, then sequence numbers get all mixed up. So we will need to ensure that older writers cannot operate on tables once they move to sequence numbers for correctness. That requires bumping the format version.

Does that reasoning sound correct, @aokolnychyi, @chenjunjiedada?

Yes, that makes sense. Bumping the format version can prevent an old writer from writing data to a table with a new version format. We may still need to consider how to handle the existing data.

So the question is do we want to allow writing from the old writer and new writer at the same time. Maybe a spark and a presto are allowed to write to the table at the same time? If we don't want to support the old writer and new writer at the same time, we can update the format version for existing data when committing.

I agree with Ryan that old writers should not be allowed to write to tables with sequence numbers. It seems like the right time to bump the format version and include new manifest types and snapshot id inheritance. Then we can safely assign 0 sequence number to all files that were written before we start assigning new ones.

aokolnychyi · 2019-11-25T10:01:02Z

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

  String manifestListLocation();
+
+  /**
+   * Return this snapshot's sequence number, or 0 if the table has no snapshot yet.


In which case will this actually return 0? If there is no snapshot, then there is no Snapshot object, right?

It would make sense to return 0 if the snapshot was written without a sequence number, and to use a primitive, right?

aokolnychyi · 2019-11-25T10:10:05Z

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

+  public void testReadSequenceNumber() {
+    long curSeqNum;
+
+    if (table.currentSnapshot() == null) {


TableTestBase always creates a fresh table for each test, so this if block seems redundant.

No, the snapshot is produced in SnapshotProducer and the SnapshotProducer has three subtypes BaseRewriteManifests, FastAppend, MergeAppend, so only when commit is triggered inside these classes, for example, append one file and commit, a new snapshot is generated. Here we just create an empty table and no data has been written yet, so the snapshot is null. This is a bit weird though.

What I mean is table.currentSnapshot() == null will be always true, so it looks like the else branch will never be executed.

aokolnychyi · 2019-11-25T10:12:24Z

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

+import org.junit.Test;
+
+
+public class TestSequenceNumber extends TableTestBase {


I think it would be great to add more tests. A few possible scenarios:

Concurrent conflicting operations

Transactions with multiple operations

Rollback

Agreed, I will add more unit tests in the next commit.

Would it also help to add a test for picked snapshots (cherry pick feature with WAP)?

Thanks @fbocse! Let me take a look for the cherry-picked snapshot feature first.

Since there are changes to commit workflow (MergingSnapshotProducer), it would be good to test commit-fail-retry scenarios like TestTransaction does.

aokolnychyi · 2019-11-25T10:21:23Z

@chenjunjiedada @rdblue do we want to add sequence numbers to manifests/files and to modify ManifestReader in a follow-up PR?

chenjunjiedada · 2019-11-25T15:44:02Z

@aokolnychyi, Thanks for your review, I will submit a commit this week to add more unit tests.

For the sequence number of Manifests and Datafile, I think the next step to add a PR to add sequence number for manifests in manifestList as Ryan commented before, while I haven't started that yet due to an internal milestone. You can go ahead if you are interested:)

aokolnychyi · 2019-11-29T18:04:54Z

@chenjunjiedada @rdblue, I spent some time prototyping how we can inherit snapshot ids and created #675 as a result. I think we simply need to make sure that logic there is flexible enough to inherit sequence numbers later on. It would be great to hear what you think.

chenjunjiedada · 2020-01-30T15:36:30Z

@aokolnychyi, @rdblue , I updated the code with the basic inheritance feature. Could you please help to have a look? Will rebase later.

chenjunjiedada · 2020-01-30T15:54:02Z

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

      OutputFile out = manifestPath(manifestCount.getAndIncrement());

-      ManifestWriter writer = new ManifestWriter(spec, out, snapshotId());
+      ManifestWriter writer = new ManifestWriter(spec, out, snapshotId(), sequenceNumber());


The sequence number may changes when the commit fails, and a sequence number is not like snapshot id that can be used in the next commit. So this logic should be wrong. We may need to store sequence numbers in the manifest list file, it seems too much for manifest list file. @rdblue @aokolnychyi what do you think?

I should misunderstand the logic, please ignore this.

chenjunjiedada · 2020-02-04T17:25:32Z

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

            }

+            // reset sequence number to null so that the sequence number can be updated when retry
+            this.sequenceNumber = null;


Because the sequence number cannot be unique, so it has to reset to null for next use.

chenjunjiedada · 2020-02-04T17:28:35Z

Hi @rdblue, @aokolnychyi, I rebased the code. Could you please help to have a look at your convenience?

aokolnychyi · 2020-02-04T19:31:09Z

Thanks, @chenjunjiedada! Will take a look today.

Conflicts: core/src/main/java/org/apache/iceberg/SnapshotProducer.java

rdblue · 2020-02-07T18:34:27Z

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

+   * @return The sequence number to identify the order in which data files and deletion files are to be processed.
+   * If the sequence number is not specified it is inherited from the manifest file struct in the manifest list file.
+   */
+  Long sequenceNumber();


Should this ever return null? I would rather use a valid number and a primitive type to ensure that we always have a sequence number that is valid. For older data, the sequence number should be 0, indicating that it is older than when we started writing sequence numbers.

I use boxing here because I don't want to rewrite the manifest file. The sequence number is not determined when committing the update, leave it to null so that we can inherit the sequence number from the value in the manifest list file.

If we use the primitive type, we need to check where the sequence number of the manifest is null or not. It should be doable, let me update this.

rdblue · 2020-02-07T18:36:13Z

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

      addedManifests.add(manifest);
    } else {
-      // the manifest must be rewritten with this update's snapshot ID
+      // the manifest must be rewritten with this update's snapshot ID and sequence number


The rewrite cannot set the sequence number. If it did, then we would have to rewrite the manifest again if the commit fails and has to retry -- because the sequence number would be incremented.

You are right. When the commit fails, it just needs to update the sequence number in the manifest list file and doesn't have to keep sequence number in the manifest file.

rdblue · 2020-02-07T18:53:22Z

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

    this.fromProjectionPos = null;
  }

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


This is an internal class, so it shouldn't be necessary to add a new constructor.

Agreed, no place uses it actually.

chenjunjiedada · 2020-02-12T15:12:35Z

@rdblue @aokolnychyi, Base on the comments, I assume the next steps are

bump the table format version
set a compatibility flag compatibility.sequence-number.enabled
update existing writers to add logic to check format version

What do you think?

chenjunjiedada · 2020-02-17T08:12:50Z

We already have the logic to check the reading format version so I just bump the version. @rdblue @aokolnychyi could you please help to take another look? Do you think this is ready?

chenjunjiedada · 2020-02-17T14:10:16Z

Hm, Looks like we need to update spec.md as well at least.

chenjunjiedada · 2020-03-03T06:37:39Z

@rdblue @aokolnychyi , Any more comments or suggestions?

Conflicts: site/docs/spec.md

openinx · 2020-03-30T03:28:12Z

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

+    return 16;
  }

  @Override


Please add the sequence_number inside the toString() ?

The GenericDataFile(GenericDataFile toCopy, boolean fullCopy) also need to copy the sequence_number I guess.

openinx

LGTM overall, left few comments. Ping @rdblue , @aokolnychyi, any other concerns ? Please help to confirm this spec again so that we can move the row-delete a bit faster, Thanks in advance.

openinx · 2020-03-30T03:50:30Z

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

+    return 16;
  }

  @Override


The GenericDataFile(GenericDataFile toCopy, boolean fullCopy) also need to copy the sequence_number I guess.

openinx · 2020-03-30T03:54:19Z

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

  private Integer deletedFilesCount = null;
  private Long deletedRowsCount = null;
  private List<PartitionFieldSummary> partitions = null;
+  private Long sequenceNumber = null;


Please also check the similar things in GenericDataFile, such as the constructor ? toString() ? etc.

openinx · 2020-03-30T04:00:23Z

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

  private long deletedRows = 0L;
+  private Long sequenceNumber = null;

  ManifestWriter(PartitionSpec spec, OutputFile file, Long snapshotId) {


nit: could write it as :

this(spec, file, snapshotId, null);

prodeezy · 2020-04-06T20:13:10Z

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

  private final InputFile manifestList;
  private final String operation;
  private final Map<String, String> summary;
+  private Long sequenceNumber;


@chenjunjiedada Can you comment on why the max seq number is not being tracked in Snapshot but instead Snapshot Seq Number is? the Merge-on-Read doc says we use max seq number in the snapshot. Maybe i missed the context. what's the reason for switching to snapshot sequence number?

@prodeezy , you may want to check this: #588 (comment).

prodeezy · 2020-04-06T20:22:21Z

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

+   * The data files' sequence number is optional, it should inherit the manifest's sequence number if the reader reads
+   * null from manifest file.
+   */
+  Long sequenceNumber();


So if a seq number is available at manifest file level then we assume all datafiles in that manifestfile have the same seq number. But if there are datafile level seq numbers then should we be tracking any metadata at ManifestFile level? Like Min/Max seq number?

Currently, the logic is just the first step that if a seq number is not found in data_file, then we try to find it from manifestFile. It doesn't contain the logic about computing the min/max value. I would like to use separated fields to track the min/max sequence number in the next step.

chenjunjiedada · 2020-04-07T08:43:02Z

@openinx @prodeezy Thanks for your review. This doesn't contain min/max sequence number computing logic right now. The plan is to add the sequence number at first since that would need some essential changes and may break compatibility like we already raised for table format. Once the sequence number is in, we will consider the optimization including min/max sequence number and etc.

prodeezy · 2020-04-07T22:18:37Z

@chenjunjiedada Like you mentioned how the Manifestfile::sequenceNumber and DataFile::sequenceNumber are related, can you document (maybe in a field comment) what their relationship is with Snapshot::sequenceNumber? is it used for higher level ordering or default values in case Manifest doesn't have a seuqnceNumber? Will Snapshots be sorted by sequence number?

chenjunjiedada · 2020-04-09T13:33:47Z

@prodeezy , There is a description in spec.md, here. I 'm OK to add the document to Snapshot API too. Is that OK for you?

chenjunjiedada · 2020-04-24T08:16:54Z

Close this because it is obsoleted.

rdblue reviewed Nov 11, 2019

View reviewed changes

chenjunjiedada force-pushed the seq branch 4 times, most recently from 705c9d6 to aff8316 Compare November 18, 2019 04:33

aokolnychyi reviewed Nov 25, 2019

View reviewed changes

chenjunjiedada changed the title ~~Add sequential number prepare for supporting row level delete (WIP)~~ [WIP] Add sequence number for supporting row level delete Dec 3, 2019

chenjunjiedada force-pushed the seq branch from 7735c05 to 6d63e1b Compare December 3, 2019 12:45

chenjunjiedada mentioned this pull request Jan 19, 2020

Add sequence numbers to metadata #358

Closed

chenjunjiedada commented Jan 31, 2020

View reviewed changes

chenjunjiedada added 2 commits February 4, 2020 09:08

Add sequence number for snapshot

d29358a

Add more unit tests

ffc849e

chenjunjiedada force-pushed the seq branch from 6fb4e07 to ffc849e Compare February 4, 2020 01:09

Add sequence number for data file and manifest file

09eb9a8

chenjunjiedada commented Feb 4, 2020

View reviewed changes

chenjunjiedada added 2 commits February 5, 2020 22:49

Merge branch 'master' into seq

8f1fd1a

Conflicts: core/src/main/java/org/apache/iceberg/SnapshotProducer.java

add unit test for cherry-pick feature

82420a2

update unit tests

9ac1143

rdblue reviewed Feb 7, 2020

View reviewed changes

chenjunjiedada added 3 commits February 8, 2020 14:00

some minor changes

7313947

fix sequence number genration for multi-thread situation

e72cc42

Merge branch 'master' into seq

f7fd87d

bump the table format version

75fa8a8

chenjunjiedada added 2 commits February 17, 2020 23:09

update spec

8ce5e75

fix words

baeffec

Merge branch 'master' into seq

f38052f

Conflicts: site/docs/spec.md

openinx reviewed Mar 30, 2020

View reviewed changes

prodeezy reviewed Apr 6, 2020

View reviewed changes

address comments

e72ce7c

chenjunjiedada mentioned this pull request Apr 10, 2020

Update Snapshot and TableMetadata with sequence numbers #910

Merged

chenjunjiedada closed this Apr 24, 2020

rdblue mentioned this pull request Apr 27, 2020

Add unit tests for sequence number #974

Merged

chenjunjiedada deleted the seq branch July 16, 2020 03:35

		import org.junit.Test;


		public class TestSequenceNumber extends TableTestBase {

[WIP] Add sequence number for supporting row level delete #588

[WIP] Add sequence number for supporting row level delete #588

Uh oh!

Conversation

chenjunjiedada commented Oct 28, 2019

Uh oh!

aokolnychyi commented Oct 29, 2019

Uh oh!

aokolnychyi commented Nov 8, 2019

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!

rdblue commented Nov 11, 2019

Uh oh!

chenjunjiedada commented Nov 15, 2019

Uh oh!

chenjunjiedada commented Nov 18, 2019

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!

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 Nov 26, 2019 • 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!

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 commented Nov 25, 2019

Uh oh!

chenjunjiedada commented Nov 25, 2019

Uh oh!

aokolnychyi commented Nov 29, 2019

Uh oh!

chenjunjiedada commented Jan 30, 2020

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!

chenjunjiedada commented Feb 4, 2020

Uh oh!

aokolnychyi commented Feb 4, 2020

Uh oh!

aokolnychyi Nov 26, 2019 •

edited

Loading