[iceberg] branch master updated: Core: Fix NPE in SnapshotUtil when parent snapshot ID is null (#6005)

[ao...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

aokolnychyi pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/iceberg.git


The following commit(s) were added to refs/heads/master by this push:
     new 36c51c1834 Core: Fix NPE in SnapshotUtil when parent snapshot ID is null (#6005)
36c51c1834 is described below

commit 36c51c1834e4f1feecdc4b677394053a05ffd672
Author: liliwei <hi...@gmail.com>
AuthorDate: Wed Oct 19 03:30:12 2022 +0800

    Core: Fix NPE in SnapshotUtil when parent snapshot ID is null (#6005)
---
 .../java/org/apache/iceberg/util/SnapshotUtil.java |   2 +-
 .../org/apache/iceberg/util/TestSnapshotUtil.java  | 158 +++++++++++++++++++++
 2 files changed, 159 insertions(+), 1 deletion(-)

diff --git a/core/src/main/java/org/apache/iceberg/util/SnapshotUtil.java b/core/src/main/java/org/apache/iceberg/util/SnapshotUtil.java
index 93880f97cb..a3492ad119 100644
--- a/core/src/main/java/org/apache/iceberg/util/SnapshotUtil.java
+++ b/core/src/main/java/org/apache/iceberg/util/SnapshotUtil.java
@@ -72,7 +72,7 @@ public class SnapshotUtil {
   public static boolean isParentAncestorOf(
       Table table, long snapshotId, long ancestorParentSnapshotId) {
     for (Snapshot snapshot : ancestorsOf(snapshotId, table::snapshot)) {
-      if (snapshot.parentId() == ancestorParentSnapshotId) {
+      if (snapshot.parentId() != null && snapshot.parentId() == ancestorParentSnapshotId) {
         return true;
       }
     }
diff --git a/core/src/test/java/org/apache/iceberg/util/TestSnapshotUtil.java b/core/src/test/java/org/apache/iceberg/util/TestSnapshotUtil.java
new file mode 100644
index 0000000000..20c1034ce6
--- /dev/null
+++ b/core/src/test/java/org/apache/iceberg/util/TestSnapshotUtil.java
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+import java.io.File;
+import java.util.List;
+import java.util.stream.StreamSupport;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Snapshot;
+import org.apache.iceberg.TestTables;
+import org.apache.iceberg.types.Types;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+public class TestSnapshotUtil {
+  @Rule public TemporaryFolder temp = new TemporaryFolder();
+
+  // Schema passed to create tables
+  public static final Schema SCHEMA =
+      new Schema(
+          required(3, "id", Types.IntegerType.get()), required(4, "data", Types.StringType.get()));
+
+  // Partition spec used to create tables
+  protected static final PartitionSpec SPEC = PartitionSpec.builderFor(SCHEMA).build();
+
+  protected File tableDir = null;
+  protected File metadataDir = null;
+  public TestTables.TestTable table = null;
+
+  static final DataFile FILE_A =
+      DataFiles.builder(SPEC)
+          .withPath("/path/to/data-a.parquet")
+          .withFileSizeInBytes(10)
+          .withRecordCount(1)
+          .build();
+
+  private long snapshotAId;
+
+  private long snapshotATimestamp;
+  private long snapshotBId;
+  private long snapshotCId;
+  private long snapshotDId;
+
+  @Before
+  public void before() throws Exception {
+    this.tableDir = temp.newFolder();
+    tableDir.delete(); // created by table create
+
+    this.metadataDir = new File(tableDir, "metadata");
+
+    this.table = TestTables.create(tableDir, "test", SCHEMA, SPEC, 2);
+    table.newFastAppend().appendFile(FILE_A).commit();
+    this.snapshotAId = table.currentSnapshot().snapshotId();
+
+    snapshotATimestamp = System.currentTimeMillis();
+
+    table.newFastAppend().appendFile(FILE_A).commit();
+    this.snapshotBId = table.currentSnapshot().snapshotId();
+
+    table.newFastAppend().appendFile(FILE_A).commit();
+    this.snapshotDId = table.currentSnapshot().snapshotId();
+
+    String branchName = "b1";
+    table.manageSnapshots().createBranch(branchName, snapshotAId).commit();
+    table.newFastAppend().appendFile(FILE_A).toBranch(branchName).commit();
+    this.snapshotCId = table.snapshot(branchName).snapshotId();
+  }
+
+  @After
+  public void cleanupTables() {
+    TestTables.clearTables();
+  }
+
+  @Test
+  public void isParentAncestorOf() {
+    Assert.assertTrue(SnapshotUtil.isParentAncestorOf(table, snapshotBId, snapshotAId));
+    Assert.assertFalse(SnapshotUtil.isParentAncestorOf(table, snapshotCId, snapshotBId));
+  }
+
+  @Test
+  public void isAncestorOf() {
+    Assert.assertTrue(SnapshotUtil.isAncestorOf(table, snapshotBId, snapshotAId));
+    Assert.assertFalse(SnapshotUtil.isAncestorOf(table, snapshotCId, snapshotBId));
+
+    Assert.assertTrue(SnapshotUtil.isAncestorOf(table, snapshotBId));
+    Assert.assertFalse(SnapshotUtil.isAncestorOf(table, snapshotCId));
+  }
+
+  @Test
+  public void currentAncestors() {
+    Iterable<Snapshot> snapshots = SnapshotUtil.currentAncestors(table);
+    expectedSnapshots(new long[] {snapshotDId, snapshotBId, snapshotAId}, snapshots);
+
+    List<Long> snapshotList = SnapshotUtil.currentAncestorIds(table);
+    Assert.assertArrayEquals(
+        new Long[] {snapshotDId, snapshotBId, snapshotAId}, snapshotList.toArray(new Long[0]));
+  }
+
+  @Test
+  public void oldestAncestor() {
+    Snapshot snapshot = SnapshotUtil.oldestAncestor(table);
+    Assert.assertEquals(snapshotAId, snapshot.snapshotId());
+
+    snapshot = SnapshotUtil.oldestAncestorOf(table, snapshotDId);
+    Assert.assertEquals(snapshotAId, snapshot.snapshotId());
+
+    snapshot = SnapshotUtil.oldestAncestorAfter(table, snapshotATimestamp);
+    Assert.assertEquals(snapshotBId, snapshot.snapshotId());
+  }
+
+  @Test
+  public void snapshotsBetween() {
+    List<Long> snapshotIdsBetween =
+        SnapshotUtil.snapshotIdsBetween(table, snapshotAId, snapshotDId);
+    Assert.assertArrayEquals(
+        new Long[] {snapshotDId, snapshotBId}, snapshotIdsBetween.toArray(new Long[0]));
+
+    Iterable<Snapshot> ancestorsBetween =
+        SnapshotUtil.ancestorsBetween(table, snapshotDId, snapshotBId);
+    expectedSnapshots(new long[] {snapshotDId}, ancestorsBetween);
+
+    ancestorsBetween = SnapshotUtil.ancestorsBetween(table, snapshotDId, snapshotCId);
+    expectedSnapshots(new long[] {snapshotDId, snapshotBId, snapshotAId}, ancestorsBetween);
+  }
+
+  private void expectedSnapshots(long[] snapshotIdExpected, Iterable<Snapshot> snapshotsActual) {
+    long[] actualSnapshots =
+        StreamSupport.stream(snapshotsActual.spliterator(), false)
+            .mapToLong(Snapshot::snapshotId)
+            .toArray();
+    Assert.assertArrayEquals(snapshotIdExpected, actualSnapshots);
+  }
+}