Split Snapshot.manifests into dataManifests and deleteManifests #1080

rdblue · 2020-05-30T00:29:19Z

This replaces all calls to Snapshot.manifests with calls to one of 3 new methods:

Snapshot.allManifests returns both delete and data manifests
Snapshot.deleteManifests returns only delete manifests
Snapshot.dataManifests returns only data manifests

Existing references mostly use either allManifests or dataManifests, depending on the context. For example, tests with assertions for the number of manifests use allManifests because the test cases should validate there are no new delete manifests, but other tests that validate rewritten manifests are deleted use dataManifests because only data manifests are rewritten and deleted.

This tries to make minimal changes that preserve the current behavior. Operations are not updated to support delete manifests (rewrite still only rewrites data manifests), but will carry through the list of delete manifests correctly.

After this commit, we should be able to work in parallel on:

Job planning with delete manifests and delete files
Updating MergingSnapshotProducer to handle delete manifests
Updating rewrite actions to handle deletes

rdblue · 2020-05-30T00:30:00Z

@prodeezy, @rymurr, here's another row-level delete PR.

rdblue · 2020-05-30T00:34:35Z

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

-import org.apache.iceberg.types.Types;
-import org.apache.iceberg.util.CharSequenceWrapper;
-
-public class FileHistory {


This is no longer needed. It was used to find all manifest entries for a file to help debug changes. Now that can be handled in parallel by the all_entries metadata table.

rdblue · 2020-05-30T00:36:47Z

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


      // when snapshot is not null
-      CloseableIterable<ManifestEntry<DataFile>> entries = new ManifestGroup(ops.io(), snapshot.manifests())
+      CloseableIterable<ManifestEntry<DataFile>> entries = new ManifestGroup(ops.io(), snapshot.dataManifests())


The contract for this class is to return matching data files, not splits. We use it in some cases to avoid newScan that will return either FileScanTask or CombinedScanTask and will emit a scan event. I think we may be able to remove it, or update it to return both data and delete files. For now, this uses dataManifests to mimic the current behavior.

rdblue · 2020-05-30T00:37:13Z

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

    Set<ManifestFile> manifests = FluentIterable
        .from(snapshots)
-        .transformAndConcat(s -> s.manifests())
+        .transformAndConcat(s -> s.dataManifests())


For now, scans continue to ignore delete manifests and delete files.

rdblue · 2020-05-30T00:38:17Z

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

      List<ManifestFile> manifests = Lists.newArrayList();
+      if (current != null) {
+        manifests.addAll(current.deleteManifests());
+      }


Carry forward delete manifests, if present.

Moved to the end of the manifest list.

rymurr

LGTM. In places I was a bit confused about the choice between reading/using dataManifest vs allManifest but I chalk that up to my level of knowledge of the code base.

rdblue · 2020-06-01T17:06:12Z

Thanks for reviewing, @rymurr! Feel free to comment on the places that don't make sense. I might have gotten them wrong. And if not, I'll at least note why I think it should be all or just data.

rymurr

Added some comments @rdblue on places where things weren't entirely clear to me.

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

rymurr · 2020-06-02T09:49:23Z

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

      Expression rowFilter = joinFilters(filters);

-      Iterable<ManifestFile> manifests = table.currentSnapshot().manifests();
+      Iterable<ManifestFile> manifests = table.currentSnapshot().dataManifests();


Is this explicitly ignoring the effect of deleted rows on partition metrics or is it just that you are short circuiting any delete files (as we can't use them anyways)

This whole class will need to be updated to support detecting changes in delete files, so I just did the minimum to update it safely. Since it uses ManifestFiles.read later instead of ManifestFiles.readDeletes, it made sense to only use data manifests. This is one that we should plan to update in a separate PR, if anyone is using it.

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

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

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

aokolnychyi · 2020-06-02T15:55:09Z

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

+   *
+   * @return a list of ManifestFile
+   */
+  List<ManifestFile> allManifests();


This seem like a breaking change to the public API. Technically, the old description already assumed a list of all manifests. Do we think it is better to change it now to avoid confusion and misuse?

The question is where we want to make the breaking changes. The format can't be read correctly without support for deletes, so to support v2 we will be making some breaking changes. If we do it with API changes then clients will need to update.

If we just return manifest files for deletes through the old manifests method, then older clients will appear to work until they encounter a delete manifest.

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

aokolnychyi · 2020-06-02T16:19:00Z

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

+    }
+
+    if (dataManifests == null) {
+      this.dataManifests = ImmutableList.copyOf(Iterables.filter(allManifests,


The list of manifests can contain 1-2k entries, will iterating through that list twice here have an impact on performance?

No, this would be pretty quick.

aokolnychyi · 2020-06-02T16:21:48Z

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

-  @Override
-  public List<ManifestFile> manifests() {
-    if (manifests == null) {
+  private void cacheManifests() {


Do we have to reason about thread safety like we do in PartitionSpec, for example?

No. The main problem in #219 was that the lazy value was set before it was completely initialized. Here, allManifests is set to a list that will not change.

In that PR, we also added double checking and a lock to prevent two threads from building the same value. We could do that here as well, but it doesn't affect correctness. There is just a race condition to produce the same sets of manifests.

I think it would make sense to prevent the race condition later on. I'll submit a PR.

aokolnychyi

+1

aokolnychyi · 2020-06-02T19:51:40Z

Thanks for the change, @rdblue! Thanks for reviewing, @rymurr!

…he#1080) This replaces all calls to Snapshot.manifests with calls to one of 3 new methods: * `Snapshot.allManifests` returns both delete and data manifests * `Snapshot.deleteManifests` returns only delete manifests * `Snapshot.dataManifests` returns only data manifests Existing references mostly use either `allManifests` or `dataManifests`, depending on the context. For example, tests with assertions for the number of manifests use `allManifests` because the test cases should validate there are no new delete manifests, but other tests that validate rewritten manifests are deleted use `dataManifests` because only data manifests are rewritten and deleted. This tries to make minimal changes that preserve the current behavior. Operations are not updated to support delete manifests (rewrite still only rewrites data manifests), but will carry through the list of delete manifests correctly.

rdblue requested review from aokolnychyi and danielcweeks May 30, 2020 00:29

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

rdblue commented May 30, 2020

View reviewed changes

Split Snapshot.manifests into dataManifests and deleteManifests.

7f3dfce

rdblue force-pushed the separate-snapshot-manifests branch from 411bd27 to 7f3dfce Compare May 30, 2020 00:53

rymurr reviewed Jun 1, 2020

View reviewed changes

Fix checkstyle.

8342d26

rdblue added 2 commits June 1, 2020 12:15

Fix test failures.

bc082c5

Fix manifest caching.

3b7f05c

rymurr reviewed Jun 2, 2020

View reviewed changes

aokolnychyi reviewed Jun 2, 2020

View reviewed changes

Update after reviews.

e565075

aokolnychyi approved these changes Jun 2, 2020

View reviewed changes

aokolnychyi merged commit 482f420 into apache:master Jun 2, 2020

rdblue mentioned this pull request Jun 3, 2020

Update sequence number tests to use allManifests #1087

Merged

Split Snapshot.manifests into dataManifests and deleteManifests #1080

Split Snapshot.manifests into dataManifests and deleteManifests #1080

Uh oh!

Conversation

rdblue commented May 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rdblue commented May 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rymurr left a comment

Choose a reason for hiding this comment

Uh oh!

rdblue commented Jun 1, 2020

Uh oh!

rymurr left a comment

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!

Uh oh!

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!

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 Jun 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rdblue commented May 30, 2020 •

edited

Loading