You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "wangyong9999 (via GitHub)" <gi...@apache.org> on 2026/09/08 13:10:10 UTC
[PR] perf(read): reuse the data scan snapshot for PK index planning [paimon-cpp]
wangyong9999 opened a new pull request, #303:
URL: https://github.com/apache/paimon-cpp/pull/303
### Purpose
No linked issue.
PK index planning first creates the data plan, whose starting scanner loads the selected snapshot. It then loads the same snapshot again before scanning index metadata. On remote storage this adds another open/read round trip, including when no matching index entries exist.
Reuse the snapshot already held by the scan when its ID matches the data plan. Otherwise retain the existing load path. This is per-scan reuse, not a shared cache; snapshot discovery and freshness are unchanged.
### Tests
- Extend the historical-snapshot integration test to count snapshot opens while preserving result checks for snapshots 2, 3 and 5.
- Local targeted executable: all 12 Parquet/ORC PK sorted-index integration cases pass. Built the changed translation unit and integration test against the matching main-based dependency bundle; this is not a full CMake build.
- Negative control using the unmodified implementation fails both format cases: two snapshot opens instead of one.
- `pre-commit run --all-files` and `git diff --check` pass. Full CI pending.
- Local `-Wall` build has signed-comparison warnings from the older bundled GoogleTest headers; no production-source warning observed.
### API and Format
Internal accessors only. No change under `include/`, storage format, or protocol.
### Documentation
No new configuration or user-facing feature.
### Generative AI tooling
Generated-by: OpenAI Codex (GPT-5)
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(read): reduce snapshot and option parsing overhead [paimon-cpp]
Posted by "lxy-9602 (via GitHub)" <gi...@apache.org>.
lxy-9602 merged PR #303:
URL: https://github.com/apache/paimon-cpp/pull/303
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(read): reuse the data scan snapshot for PK index planning [paimon-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(read): reuse the data scan snapshot for PK index planning [paimon-cpp]
Posted by "wangyong9999 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org