Copilot commented on code in PR #303:
URL: https://github.com/apache/paimon-cpp/pull/303#discussion_r3958490468
##########
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:
Returning a `const std::optional<Snapshot>&` exposes internal storage and
encourages consumers to couple to the exact representation (optional + value
semantics). If this is intended to be read-only access for reuse, consider
returning a narrower view (e.g., `const Snapshot*` or
`std::optional<std::reference_wrapper<const Snapshot>>`) to reduce coupling and
make it harder for callers to accidentally depend on object
lifetime/representation details.
##########
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 reuse path still copies the `Snapshot` into a new `Result<Snapshot>`. If
`Snapshot` is non-trivial to copy (or becomes larger over time), this erodes
the benefit of reuse. Consider a non-copying design for this flow (e.g.,
passing around `const Snapshot*`, `std::shared_ptr<const Snapshot>`, or an
`optional<reference_wrapper<const Snapshot>>`), or adding a `Result` helper
that can wrap a reference/ptr for read-only use during planning.
##########
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 `std::vector<T>::difference_type` (typically a signed
integral) or effectively `size_t` in common usage; returning `int64_t` here
introduces an implicit conversion. Consider returning `size_t` (or explicitly
casting) to avoid signed/unsigned and narrowing concerns in comparisons and
future refactors.
##########
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:
This assertion is fragile because it relies on an exact path string match.
If snapshot loading changes to use a different path representation (absolute vs
relative, normalized separators) or a filename convention (e.g., extensions),
the test may fail even though behavior is correct. Consider normalizing paths
before recording/counting, or counting opens using a predicate (e.g.,
suffix/prefix/basename match) that reflects the semantic intent (‘snapshot <id>
opened exactly once’).
--
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]
