wangyong9999 commented on code in PR #303:
URL: https://github.com/apache/paimon-cpp/pull/303#discussion_r3958711857
##########
src/paimon/core/operation/file_store_scan.h:
##########
@@ -108,6 +108,10 @@ class FileStoreScan {
return this;
}
+ const std::optional<Snapshot>& GetSpecifiedSnapshot() const {
+ return specified_snapshot_;
Review Comment:
This is an internal accessor, consumed synchronously while SnapshotReader
owns the scan. The reference is not retained. I kept the existing optional
representation; a pointer would have the same lifetime constraint here.
##########
src/paimon/core/table/source/primary_key_index_batch_scan.cpp:
##########
@@ -86,7 +86,11 @@ Result<std::shared_ptr<Plan>>
PrimaryKeyIndexBatchScan::CreatePlan() {
int64_t snapshot_id = data_plan->SnapshotId().value();
const std::shared_ptr<SnapshotManager>& snapshot_manager =
snapshot_reader_->GetSnapshotManager();
- Result<Snapshot> snapshot_result =
snapshot_manager->LoadSnapshot(snapshot_id);
+ // The data scan already loaded this snapshot. Reuse it for index planning.
+ const std::optional<Snapshot>& planned_snapshot =
snapshot_reader_->GetSpecifiedSnapshot();
+ Result<Snapshot> snapshot_result = planned_snapshot &&
planned_snapshot->Id() == snapshot_id
+ ?
Result<Snapshot>(planned_snapshot.value())
+ :
snapshot_manager->LoadSnapshot(snapshot_id);
Review Comment:
The copy contains snapshot metadata, not manifest entries or file data. This
change targets the duplicate remote open/read and JSON parsing. I would keep
the existing Result flow here rather than add shared ownership or
reference-result machinery without a measured need.
##########
test/inte/primary_key_sorted_index_inte_test.cpp:
##########
@@ -292,11 +297,15 @@ TEST_P(PrimaryKeySortedIndexInteTest,
HistoricalSnapshotsMixedFilesAndDisabledIn
std::shared_ptr<Predicate> predicate = ScoreEqual(/*partitioned=*/false,
0);
const std::vector<int64_t> snapshot_ids = {2, 3, 5};
for (int64_t snapshot_id : snapshot_ids) {
+ auto tracking_file_system =
std::make_shared<TrackingLocalFileSystem>();
ASSERT_OK_AND_ASSIGN(
std::shared_ptr<Plan> plan,
Scan(/*partitioned=*/false, predicate, snapshot_id,
/*index_enabled=*/true,
- /*partition_filters=*/{}, /*branch=*/"",
/*file_system=*/nullptr));
+ /*partition_filters=*/{}, /*branch=*/"",
tracking_file_system));
ASSERT_EQ(snapshot_id, plan->SnapshotId());
+ ASSERT_EQ(1, tracking_file_system->OpenCount(
+ PathUtil::JoinPath(TablePath(/*partitioned=*/false),
+
fmt::format("snapshot/snapshot-{}", snapshot_id))));
Review Comment:
The expected path is built from the same fixture table path using
PathUtil::JoinPath. Exact matching also prevents counting another snapshot or
branch. The Parquet and ORC cases pass, and the unmodified implementation fails
with two opens, so I will keep this assertion.
##########
test/inte/primary_key_sorted_index_inte_test.cpp:
##########
@@ -75,6 +75,11 @@ class TrackingLocalFileSystem : public LocalFileSystem {
return index_paths;
}
+ int64_t OpenCount(const std::string& path) const {
Review Comment:
std::count returns a signed difference_type, which is 64-bit on the
supported targets. This test tracks only a handful of opens, so there is no
narrowing risk here. Keeping the helper as is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
