Skip to content

Commit 48738fc

Browse files
zhjwpkuzhjwpku
authored
refactor: rename SnapshotCache to SnapshotReader (#945)
Name the class after its role of reading a snapshot's manifests, and document that it loads and caches the manifest list on first access. Update call sites and local variable names to match, preserving behavior.
1 parent 35c5df1 commit 48738fc

13 files changed

Lines changed: 101 additions & 100 deletions

‎src/iceberg/snapshot.cc‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,7 @@ Result<std::unique_ptr<Snapshot>> Snapshot::Make(
203203
});
204204
}
205205

206-
Result<SnapshotCache::ManifestsCache> SnapshotCache::InitManifestsCache(
206+
Result<SnapshotReader::ManifestsCache> SnapshotReader::InitManifestsCache(
207207
const Snapshot* snapshot, std::shared_ptr<FileIO> file_io) {
208208
if (file_io == nullptr) {
209209
return InvalidArgument("Cannot cache manifests: FileIO is null");
@@ -236,21 +236,21 @@ Result<SnapshotCache::ManifestsCache> SnapshotCache::InitManifestsCache(
236236
return std::make_pair(std::move(manifests), data_manifests_count);
237237
}
238238

239-
Result<std::span<ManifestFile>> SnapshotCache::Manifests(
239+
Result<std::span<ManifestFile>> SnapshotReader::Manifests(
240240
std::shared_ptr<FileIO> file_io) const {
241241
ICEBERG_ASSIGN_OR_RAISE(auto cache_ref, manifests_cache_.Get(snapshot_, file_io));
242242
auto& cache = cache_ref.get();
243243
return std::span<ManifestFile>(cache.first.data(), cache.first.size());
244244
}
245245

246-
Result<std::span<ManifestFile>> SnapshotCache::DataManifests(
246+
Result<std::span<ManifestFile>> SnapshotReader::DataManifests(
247247
std::shared_ptr<FileIO> file_io) const {
248248
ICEBERG_ASSIGN_OR_RAISE(auto cache_ref, manifests_cache_.Get(snapshot_, file_io));
249249
auto& cache = cache_ref.get();
250250
return std::span<ManifestFile>(cache.first.data(), cache.second);
251251
}
252252

253-
Result<std::span<ManifestFile>> SnapshotCache::DeleteManifests(
253+
Result<std::span<ManifestFile>> SnapshotReader::DeleteManifests(
254254
std::shared_ptr<FileIO> file_io) const {
255255
ICEBERG_ASSIGN_OR_RAISE(auto cache_ref, manifests_cache_.Get(snapshot_, file_io));
256256
auto& cache = cache_ref.get();

‎src/iceberg/snapshot.h‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -462,12 +462,11 @@ struct ICEBERG_EXPORT Snapshot {
462462
bool Equals(const Snapshot& other) const;
463463
};
464464

465-
/// \brief A snapshot with cached manifest loading capabilities.
466-
///
467-
/// This class wraps a Snapshot pointer and provides lazy-loading of manifests.
468-
class ICEBERG_EXPORT SnapshotCache {
465+
/// \brief Reads a snapshot's manifests, loading and caching its manifest list on
466+
/// first access.
467+
class ICEBERG_EXPORT SnapshotReader {
469468
public:
470-
explicit SnapshotCache(const Snapshot* snapshot) : snapshot_(snapshot) {}
469+
explicit SnapshotReader(const Snapshot* snapshot) : snapshot_(snapshot) {}
471470

472471
/// \brief Get the underlying Snapshot reference
473472
const Snapshot& snapshot() const { return *snapshot_; }

‎src/iceberg/table_scan.cc‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -681,9 +681,9 @@ Result<FileScanTaskStreamPtr> DataTableScan::PlanFilesStream() const {
681681
TableMetadataCache metadata_cache(metadata_.get());
682682
ICEBERG_ASSIGN_OR_RAISE(auto specs_by_id, metadata_cache.GetPartitionSpecsById());
683683

684-
SnapshotCache snapshot_cache(snapshot.get());
685-
ICEBERG_ASSIGN_OR_RAISE(auto data_manifests, snapshot_cache.DataManifests(io_));
686-
ICEBERG_ASSIGN_OR_RAISE(auto delete_manifests, snapshot_cache.DeleteManifests(io_));
684+
SnapshotReader snapshot_reader(snapshot.get());
685+
ICEBERG_ASSIGN_OR_RAISE(auto data_manifests, snapshot_reader.DataManifests(io_));
686+
ICEBERG_ASSIGN_OR_RAISE(auto delete_manifests, snapshot_reader.DeleteManifests(io_));
687687

688688
if (scan_metrics) {
689689
scan_metrics->total_data_manifests->Increment(
@@ -802,8 +802,8 @@ Result<std::vector<std::shared_ptr<FileScanTask>>> IncrementalAppendScan::PlanFi
802802

803803
std::unordered_set<ManifestFile> data_manifests;
804804
for (const auto& snapshot : append_snapshots) {
805-
SnapshotCache snapshot_cache(snapshot.get());
806-
ICEBERG_ASSIGN_OR_RAISE(auto manifests, snapshot_cache.DataManifests(io_));
805+
SnapshotReader snapshot_reader(snapshot.get());
806+
ICEBERG_ASSIGN_OR_RAISE(auto manifests, snapshot_reader.DataManifests(io_));
807807
std::ranges::copy_if(manifests, std::inserter(data_manifests, data_manifests.end()),
808808
[&snapshot_ids](const ManifestFile& manifest) {
809809
return snapshot_ids.contains(manifest.added_snapshot_id);
@@ -866,20 +866,20 @@ IncrementalChangelogScan::PlanFiles(std::optional<int64_t> from_snapshot_id_excl
866866
SnapshotUtil::AncestorsBetween(*metadata_, to_snapshot_id_inclusive,
867867
from_snapshot_id_exclusive));
868868

869-
std::vector<std::pair<std::shared_ptr<Snapshot>, std::unique_ptr<SnapshotCache>>>
869+
std::vector<std::pair<std::shared_ptr<Snapshot>, std::unique_ptr<SnapshotReader>>>
870870
changelog_snapshots;
871871

872872
for (const auto& snapshot : std::ranges::reverse_view(ancestors_snapshots)) {
873873
auto operation = snapshot->Operation();
874874
if (!operation.has_value() || operation.value() != DataOperation::kReplace) {
875-
auto snapshot_cache = std::make_unique<SnapshotCache>(snapshot.get());
875+
auto snapshot_reader = std::make_unique<SnapshotReader>(snapshot.get());
876876
ICEBERG_ASSIGN_OR_RAISE(auto delete_manifests,
877-
snapshot_cache->DeleteManifests(io_));
877+
snapshot_reader->DeleteManifests(io_));
878878
if (!delete_manifests.empty()) {
879879
return NotSupported(
880880
"Delete files are currently not supported in changelog scans");
881881
}
882-
changelog_snapshots.emplace_back(snapshot, std::move(snapshot_cache));
882+
changelog_snapshots.emplace_back(snapshot, std::move(snapshot_reader));
883883
}
884884
}
885885
if (changelog_snapshots.empty()) {

‎src/iceberg/test/fast_append_test.cc‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -169,8 +169,8 @@ class FastAppendTest : public UpdateTestBase {
169169

170170
Result<std::vector<ManifestFile>> CurrentDataManifests() {
171171
ICEBERG_ASSIGN_OR_RAISE(auto snapshot, table_->current_snapshot());
172-
SnapshotCache snapshot_cache(snapshot.get());
173-
ICEBERG_ASSIGN_OR_RAISE(auto manifests, snapshot_cache.DataManifests(file_io_));
172+
SnapshotReader snapshot_reader(snapshot.get());
173+
ICEBERG_ASSIGN_OR_RAISE(auto manifests, snapshot_reader.DataManifests(file_io_));
174174
return std::vector<ManifestFile>(manifests.begin(), manifests.end());
175175
}
176176

@@ -383,8 +383,8 @@ TEST_F(FastAppendTest, CommitFailureIgnoresCleanupDeleteFailure) {
383383
});
384384
ASSERT_THAT(append->Commit(), IsOk());
385385
ICEBERG_UNWRAP_OR_FAIL(auto snapshot, txn->current().Snapshot());
386-
SnapshotCache cache(snapshot.get());
387-
ICEBERG_UNWRAP_OR_FAIL(auto manifests, cache.Manifests(file_io_));
386+
SnapshotReader snapshot_reader(snapshot.get());
387+
ICEBERG_UNWRAP_OR_FAIL(auto manifests, snapshot_reader.Manifests(file_io_));
388388
ASSERT_THAT(manifests, ::testing::SizeIs(1));
389389
EXPECT_TRUE(deleted_paths.empty());
390390

@@ -448,8 +448,8 @@ TEST_F(FastAppendTest, RebaseCopiesAppendManifestAgain) {
448448
ASSERT_THAT(properties->Commit(), IsOk());
449449
}
450450
ICEBERG_UNWRAP_OR_FAIL(auto snapshot, txn->current().Snapshot());
451-
SnapshotCache cache(snapshot.get());
452-
ICEBERG_UNWRAP_OR_FAIL(auto manifests, cache.DataManifests(file_io_));
451+
SnapshotReader snapshot_reader(snapshot.get());
452+
ICEBERG_UNWRAP_OR_FAIL(auto manifests, snapshot_reader.DataManifests(file_io_));
453453
ASSERT_THAT(manifests, ::testing::SizeIs(1));
454454
const auto& manifest_path = manifests[0].manifest_path;
455455
EXPECT_NE(manifest_path, path);
@@ -1076,8 +1076,8 @@ TEST_F(FastAppendTest, TransientConflictThenUnknownPreservesInitialAttempt) {
10761076
EXPECT_TRUE(deleted_paths.empty());
10771077
ICEBERG_UNWRAP_OR_FAIL(auto snapshot, txn->current().Snapshot());
10781078
EXPECT_THAT(file_io_->ReadFile(snapshot->manifest_list, std::nullopt), IsOk());
1079-
SnapshotCache cache(snapshot.get());
1080-
ICEBERG_UNWRAP_OR_FAIL(auto manifests, cache.Manifests(file_io_));
1079+
SnapshotReader snapshot_reader(snapshot.get());
1080+
ICEBERG_UNWRAP_OR_FAIL(auto manifests, snapshot_reader.Manifests(file_io_));
10811081
ASSERT_EQ(manifests.size(), 1U);
10821082
EXPECT_THAT(file_io_->ReadFile(manifests[0].manifest_path, std::nullopt), IsOk());
10831083
EXPECT_THAT(txn->Abort(), IsError(ErrorKind::kValidationFailed));
@@ -1323,8 +1323,8 @@ TEST_F(FastAppendTest, ReplayFailurePartwayThroughCleansAllUncommittedGeneration
13231323
auto metadata = ReloadMetadata();
13241324
for (const auto& snapshot : metadata->snapshots) {
13251325
committed_paths.insert(snapshot->manifest_list);
1326-
SnapshotCache cache(snapshot.get());
1327-
ICEBERG_UNWRAP_OR_FAIL(auto manifests, cache.Manifests(file_io_));
1326+
SnapshotReader snapshot_reader(snapshot.get());
1327+
ICEBERG_UNWRAP_OR_FAIL(auto manifests, snapshot_reader.Manifests(file_io_));
13281328
for (const auto& manifest : manifests) {
13291329
committed_paths.insert(manifest.manifest_path);
13301330
}

‎src/iceberg/test/incremental_changelog_scan_test.cc‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,8 @@ TEST_P(IncrementalChangelogScanTest, DataFilters) {
8282
auto snapshot_a = MakeAppendSnapshotWithPartitionValues(
8383
version, 1000L, std::nullopt, 1L, {{"/path/to/file_a.parquet", partition_a}},
8484
partitioned_spec_);
85-
SnapshotCache cache_a(snapshot_a.get());
86-
ICEBERG_UNWRAP_OR_FAIL(auto manifests_a, cache_a.DataManifests(file_io_));
85+
SnapshotReader reader_a(snapshot_a.get());
86+
ICEBERG_UNWRAP_OR_FAIL(auto manifests_a, reader_a.DataManifests(file_io_));
8787
ASSERT_EQ(manifests_a.size(), 1);
8888
const auto& manifest_a = manifests_a[0];
8989

@@ -176,8 +176,8 @@ TEST_P(IncrementalChangelogScanTest, DuplicatedManifests) {
176176
// Create initial snapshot_a with file_a and extract its manifest
177177
auto snapshot_a =
178178
MakeAppendSnapshot(version, 1000L, std::nullopt, 1L, {"/path/to/file_a.parquet"});
179-
SnapshotCache cache_a(snapshot_a.get());
180-
ICEBERG_UNWRAP_OR_FAIL(auto manifests_a, cache_a.DataManifests(file_io_));
179+
SnapshotReader reader_a(snapshot_a.get());
180+
ICEBERG_UNWRAP_OR_FAIL(auto manifests_a, reader_a.DataManifests(file_io_));
181181
ASSERT_EQ(manifests_a.size(), 1);
182182
auto manifest_a = manifests_a[0];
183183

‎src/iceberg/test/merge_append_test.cc‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -278,8 +278,8 @@ class MergeAppendTestBase : public MinimalUpdateTestBase {
278278

279279
Result<std::vector<ManifestFile>> DataManifests(
280280
const std::shared_ptr<Snapshot>& snapshot) {
281-
SnapshotCache snapshot_cache(snapshot.get());
282-
ICEBERG_ASSIGN_OR_RAISE(auto manifests, snapshot_cache.DataManifests(file_io_));
281+
SnapshotReader snapshot_reader(snapshot.get());
282+
ICEBERG_ASSIGN_OR_RAISE(auto manifests, snapshot_reader.DataManifests(file_io_));
283283
return std::vector<ManifestFile>(manifests.begin(), manifests.end());
284284
}
285285

@@ -555,8 +555,8 @@ TEST_P(MergeAppendTest, EmptyTableAppendFilesWithDifferentSpecs) {
555555

556556
EXPECT_THAT(table_->Refresh(), IsOk());
557557
ICEBERG_UNWRAP_OR_FAIL(auto snapshot, CurrentSnapshot());
558-
SnapshotCache snapshot_cache(snapshot.get());
559-
ICEBERG_UNWRAP_OR_FAIL(auto data_manifests, snapshot_cache.DataManifests(file_io_));
558+
SnapshotReader snapshot_reader(snapshot.get());
559+
ICEBERG_UNWRAP_OR_FAIL(auto data_manifests, snapshot_reader.DataManifests(file_io_));
560560
std::vector<ManifestFile> manifest_files(data_manifests.begin(), data_manifests.end());
561561
ASSERT_EQ(manifest_files.size(), 2U);
562562

@@ -648,8 +648,8 @@ TEST_P(MergeAppendTest, MergeWithAppendFilesAndManifest) {
648648

649649
EXPECT_THAT(table_->Refresh(), IsOk());
650650
ICEBERG_UNWRAP_OR_FAIL(auto snapshot, CurrentSnapshot());
651-
SnapshotCache snapshot_cache(snapshot.get());
652-
ICEBERG_UNWRAP_OR_FAIL(auto data_manifests, snapshot_cache.DataManifests(file_io_));
651+
SnapshotReader snapshot_reader(snapshot.get());
652+
ICEBERG_UNWRAP_OR_FAIL(auto data_manifests, snapshot_reader.DataManifests(file_io_));
653653
ASSERT_EQ(data_manifests.size(), 1U);
654654
EXPECT_NE(data_manifests[0].manifest_path, path);
655655

@@ -919,9 +919,9 @@ TEST_P(MergeAppendTest, MinMergeCount) {
919919
EXPECT_THAT(append_c->Commit(), IsOk());
920920
EXPECT_THAT(table_->Refresh(), IsOk());
921921
ICEBERG_UNWRAP_OR_FAIL(auto snapshot_before_merge, CurrentSnapshot());
922-
SnapshotCache before_cache(snapshot_before_merge.get());
922+
SnapshotReader before_reader(snapshot_before_merge.get());
923923
ICEBERG_UNWRAP_OR_FAIL(auto manifests_before_merge,
924-
before_cache.DataManifests(file_io_));
924+
before_reader.DataManifests(file_io_));
925925
EXPECT_EQ(manifests_before_merge.size(), 3U);
926926

927927
ICEBERG_UNWRAP_OR_FAIL(auto append_d, NewBranchMergeAppend());

0 commit comments

Comments
 (0)