lxy-9602 commented on code in PR #258:
URL: https://github.com/apache/paimon-cpp/pull/258#discussion_r3911639578
##########
src/paimon/core/table/source/data_evolution_batch_scan.cpp:
##########
@@ -44,27 +90,43 @@ DataEvolutionBatchScan::DataEvolutionBatchScan(
executor_(executor) {}
Result<std::shared_ptr<Plan>> DataEvolutionBatchScan::CreatePlan() {
- std::optional<std::vector<Range>> row_ranges;
+ std::optional<int64_t> global_index_snapshot_id;
std::shared_ptr<GlobalIndexResult> final_global_index_result =
global_index_result_;
if (!final_global_index_result) {
- PAIMON_ASSIGN_OR_RAISE(std::shared_ptr<GlobalIndexResult>
index_result, EvalGlobalIndex());
- if (index_result) {
- final_global_index_result = index_result;
- PAIMON_ASSIGN_OR_RAISE(row_ranges, index_result->ToRanges());
+ PAIMON_ASSIGN_OR_RAISE(std::optional<EvaluatedGlobalIndex>
evaluated_index,
+ EvalGlobalIndex());
+ if (evaluated_index) {
+ final_global_index_result = evaluated_index->result;
+ global_index_snapshot_id = evaluated_index->snapshot_id;
}
- } else {
- PAIMON_ASSIGN_OR_RAISE(row_ranges,
final_global_index_result->ToRanges());
}
- if (!row_ranges) {
+ if (!final_global_index_result) {
return batch_scan_->CreatePlan();
}
- if (row_ranges.value().empty()) {
- return PlanImpl::EmptyPlan();
+ if (UsesUnsupportedTimeTravel(core_options_)) {
+ return Status::NotImplemented("Global index scan does not support time
travel");
}
- PAIMON_ASSIGN_OR_RAISE(RowRangeIndex row_range_index,
- RowRangeIndex::Create(row_ranges.value()));
+ PAIMON_ASSIGN_OR_RAISE(std::vector<Range> row_ranges,
final_global_index_result->ToRanges());
+ if (row_ranges.empty()) {
+ if (!global_index_snapshot_id) {
+ const std::shared_ptr<SnapshotManager>& snapshot_manager =
+ snapshot_reader_->GetSnapshotManager();
+ PAIMON_ASSIGN_OR_RAISE(std::optional<Snapshot> snapshot,
+
ResolveGlobalIndexScanSnapshot(core_options_, snapshot_manager));
+ if (!snapshot) {
Review Comment:
If `global_index_result_` is set externally, how do we infer its snapshot
ID? In that case, using `ResolveGlobalIndexScanSnapshot` does not seem very
appropriate.
I’d suggest being stricter here: if predicates are present with global
index, we should either reject time travel options such as timestamp/tag, or
properly implement the corresponding support like java, rather than adding
defensive checks in multiple places.
--
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]
