Skip to content

Commit ecf7262

Browse files
committed
fix: allow SnapshotUtil::IsAncestorOf to handle an empty current snapshot
metadata.Snapshot() raised "No current snapshot" when the ancestor check had nothing to compare against, which surfaced as a cherry-pick failure when picking onto a branch with no history yet. Java's equivalent (SnapshotUtil.ancestorsOf(null, ...)) treats this as an empty ancestor list instead of an error, so nothing can be an ancestor of it; mirror that here.
1 parent a79f110 commit ecf7262

3 files changed

Lines changed: 53 additions & 3 deletions

File tree

‎src/iceberg/test/cherry_pick_operation_test.cc‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,19 @@ class CherryPickOperationTest : public UpdateTestBase {
114114
return snap.value()->snapshot_id;
115115
}
116116

117+
// Commit an append onto the given branch and return its snapshot ID. Main's
118+
// current snapshot is unchanged.
119+
int64_t CommitAppendToBranch(const std::string& branch,
120+
const std::shared_ptr<DataFile>& file) {
121+
auto fa = table_->NewFastAppend();
122+
EXPECT_TRUE(fa.has_value());
123+
fa.value()->ToBranch(branch);
124+
fa.value()->AppendFile(file);
125+
EXPECT_THAT(fa.value()->Commit(), IsOk());
126+
EXPECT_THAT(table_->Refresh(), IsOk());
127+
return table_->metadata()->snapshots.back()->snapshot_id;
128+
}
129+
117130
// Commit a staged append and return the staged snapshot's ID. The table's
118131
// current snapshot is unchanged.
119132
int64_t StageAppend(const std::shared_ptr<DataFile>& file,
@@ -280,6 +293,26 @@ TEST_F(CherryPickOperationTest, CherryPickAppend) {
280293
replacement_a_->file_path));
281294
}
282295

296+
// A snapshot picked from another branch of an otherwise empty table can be
297+
// applied to main even though main has no current snapshot yet. The pick's
298+
// parent (on the source branch) keeps it from qualifying as a fast-forward, so
299+
// it must go through the ordinary ancestor checks with no current snapshot to
300+
// compare against.
301+
TEST_F(CherryPickOperationTest, CherryPickAppendOntoEmptyMain) {
302+
CommitAppendToBranch("b1", file_a_);
303+
int64_t staged_id = CommitAppendToBranch("b1", file_b_);
304+
305+
EXPECT_THAT(Cherrypick(staged_id), IsOk());
306+
307+
ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot());
308+
EXPECT_NE(snapshot->snapshot_id, staged_id);
309+
EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kSourceSnapshotId),
310+
std::to_string(staged_id));
311+
312+
ICEBERG_UNWRAP_OR_FAIL(auto live, LiveDataFilePaths());
313+
EXPECT_THAT(live, ::testing::UnorderedElementsAre(file_b_->file_path));
314+
}
315+
283316
// When the picked snapshot's parent is the current snapshot, the pick moves the
284317
// current snapshot instead of creating one.
285318
TEST_F(CherryPickOperationTest, FastForwardSetsCurrentSnapshot) {

‎src/iceberg/test/snapshot_util_test.cc‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,19 @@ TEST_F(SnapshotUtilTest, IsAncestorOf) {
222222
EXPECT_FALSE(result5);
223223
}
224224

225+
// No current snapshot means an empty ancestor history, so nothing can be an
226+
// ancestor of it; this must return false rather than raising "No current
227+
// snapshot", matching Java's SnapshotUtil.ancestorsOf(null, ...).
228+
TEST_F(SnapshotUtilTest, IsAncestorOfNoCurrentSnapshot) {
229+
TableMetadata metadata;
230+
metadata.current_snapshot_id = kInvalidSnapshotId;
231+
metadata.snapshots = {CreateSnapshot(base_snapshot_id_, std::nullopt, 1, base_timestamp_)};
232+
233+
ICEBERG_UNWRAP_OR_FAIL(auto result,
234+
SnapshotUtil::IsAncestorOf(metadata, base_snapshot_id_));
235+
EXPECT_FALSE(result);
236+
}
237+
225238
TEST_F(SnapshotUtilTest, CurrentAncestors) {
226239
ICEBERG_UNWRAP_OR_FAIL(auto ancestors, SnapshotUtil::CurrentAncestors(*table_));
227240
auto ids = ExtractSnapshotIds(ancestors);

‎src/iceberg/util/snapshot_util.cc‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,13 @@ Result<bool> SnapshotUtil::IsAncestorOf(const Table& table,
6666

6767
Result<bool> SnapshotUtil::IsAncestorOf(const TableMetadata& metadata,
6868
int64_t ancestor_snapshot_id) {
69-
ICEBERG_ASSIGN_OR_RAISE(auto current, metadata.Snapshot());
70-
ICEBERG_CHECK(current != nullptr, "Current snapshot is null");
71-
return IsAncestorOf(metadata, current->snapshot_id, ancestor_snapshot_id);
69+
// No current snapshot means an empty ancestor history, so nothing can be an
70+
// ancestor of it; matches Java's SnapshotUtil.ancestorsOf(null, ...), which
71+
// returns an empty iterable instead of raising.
72+
if (metadata.current_snapshot_id == kInvalidSnapshotId) {
73+
return false;
74+
}
75+
return IsAncestorOf(metadata, metadata.current_snapshot_id, ancestor_snapshot_id);
7276
}
7377

7478
bool SnapshotUtil::CanFastForward(const TableMetadata& metadata,

0 commit comments

Comments
 (0)