Re: [PR] fix(scan): preserve snapshot id for empty data evolution index plans [paimon-cpp]

["wangyong9999 (via GitHub)" <gi...@apache.org>]

wangyong9999 commented on code in PR #258:
URL: https://github.com/apache/paimon-cpp/pull/258#discussion_r3902904654


##########
src/paimon/core/table/source/data_evolution_batch_scan.cpp:
##########
@@ -134,27 +144,41 @@ Result<std::shared_ptr<Plan>> DataEvolutionBatchScan::WrapToIndexedSplits(
     return std::make_shared<PlanImpl>(data_plan->SnapshotId(), indexed_splits);
 }
 
-Result<std::shared_ptr<GlobalIndexResult>> DataEvolutionBatchScan::EvalGlobalIndex() const {
+Result<std::optional<DataEvolutionBatchScan::EvaluatedGlobalIndex>>
+DataEvolutionBatchScan::EvalGlobalIndex() const {
     auto predicate = batch_scan_->GetNonPartitionPredicate();
     if (!predicate) {
-        return std::shared_ptr<GlobalIndexResult>(nullptr);
+        return std::optional<EvaluatedGlobalIndex>();
     }
     if (!core_options_.GlobalIndexEnabled()) {
-        return std::shared_ptr<GlobalIndexResult>(nullptr);
+        return std::optional<EvaluatedGlobalIndex>();
     }
     auto partition_filter = batch_scan_->GetPartitionPredicate();
     // TODO(lisizhuo.lsz): support time travel
+    PAIMON_ASSIGN_OR_RAISE(std::optional<int64_t> snapshot_id, ResolveGlobalIndexSnapshotId());
+    if (!snapshot_id) {
+        return Status::Invalid("not found latest snapshot");
+    }
     PAIMON_ASSIGN_OR_RAISE(
         std::unique_ptr<GlobalIndexScan> index_scan,
-        GlobalIndexScan::Create(table_path_, core_options_.GetScanSnapshotId(), partition_filter,
-                                core_options_.ToMap(), core_options_.GetFileSystem(), executor_,
-                                pool_));
+        GlobalIndexScan::Create(table_path_, snapshot_id, partition_filter, core_options_.ToMap(),
+                                core_options_.GetFileSystem(), executor_, pool_));
     auto index_scan_impl = dynamic_cast<GlobalIndexScanImpl*>(index_scan.get());
     if (!index_scan_impl) {
         return Status::Invalid("invalid GlobalIndexScan, cannot cast to GlobalIndexScanImpl");
     }
 
-    return index_scan_impl->Scan(predicate);
+    PAIMON_ASSIGN_OR_RAISE(std::shared_ptr<GlobalIndexResult> result,
+                           index_scan_impl->Scan(predicate));
+    return std::optional<EvaluatedGlobalIndex>(EvaluatedGlobalIndex{result, snapshot_id.value()});
+}
+
+Result<std::optional<int64_t>> DataEvolutionBatchScan::ResolveGlobalIndexSnapshotId() const {
+    std::optional<int64_t> snapshot_id = core_options_.GetScanSnapshotId();
+    if (snapshot_id) {
+        return snapshot_id;
+    }

Review Comment:
   Thanks, good catches — addressed in aeeb00ac.
   
   The pre-supplied empty-result fast path now loads the resolved snapshot through `SnapshotManager` before constructing the empty plan. Nonexistent, expired, unreadable, or malformed snapshots therefore propagate an error, while a table with no snapshot still returns the null-snapshot empty plan. The regression coverage includes both snapshot 999 and a previously valid snapshot whose file was removed.
   
   Tag- and timestamp-based time travel now returns a single `NotImplemented` error (`Global index scan does not support time travel`) once planning actually obtains a Global Index result. This covers internally evaluated results and pre-supplied nonempty results, while preserving the ordinary batch-scan fallback when no usable index exists. The tests cover both options and the unindexed fallback boundary.



-- 
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