From 858f8162fd0474bf9dfd8e32b4d9f672eac2a619 Mon Sep 17 00:00:00 2001 From: manuzhang Date: Thu, 6 Aug 2026 11:56:00 +0800 Subject: [PATCH 01/21] feat: add lazy scan planning iterators Co-authored-by: Codex --- example/demo_example.cc | 16 +- src/iceberg/manifest/manifest_group.cc | 238 ++++++++++++++++++ src/iceberg/manifest/manifest_group.h | 13 + src/iceberg/manifest/manifest_reader.cc | 224 ++++++++++++----- src/iceberg/manifest/manifest_reader.h | 16 +- .../manifest/manifest_reader_internal.h | 10 +- src/iceberg/table_scan.cc | 191 +++++++++++--- src/iceberg/table_scan.h | 8 + src/iceberg/test/manifest_reader_test.cc | 31 +++ src/iceberg/test/table_scan_test.cc | 12 + 10 files changed, 657 insertions(+), 102 deletions(-) diff --git a/example/demo_example.cc b/example/demo_example.cc index 22ecc0c90..a477e5af1 100644 --- a/example/demo_example.cc +++ b/example/demo_example.cc @@ -79,7 +79,7 @@ int main(int argc, char** argv) { } auto scan = std::move(scan_result.value()); - auto plan_result = scan->PlanFiles(); + auto plan_result = scan->PlanFilesIterator(); if (!plan_result.has_value()) { std::cerr << "Failed to plan files: " << plan_result.error().message << std::endl; return 1; @@ -87,8 +87,18 @@ int main(int argc, char** argv) { std::cout << "Scan tasks: " << std::endl; auto scan_tasks = std::move(plan_result.value()); - for (const auto& scan_task : scan_tasks) { - std::cout << " - " << scan_task->data_file()->file_path << std::endl; + while (true) { + auto task_result = scan_tasks->Next(); + if (!task_result.has_value()) { + std::cerr << "Failed to plan next file: " << task_result.error().message + << std::endl; + return 1; + } + if (!task_result.value().has_value()) { + break; + } + std::cout << " - " << task_result.value().value()->data_file()->file_path + << std::endl; } return 0; diff --git a/src/iceberg/manifest/manifest_group.cc b/src/iceberg/manifest/manifest_group.cc index 02a51b113..327a19db6 100644 --- a/src/iceberg/manifest/manifest_group.cc +++ b/src/iceberg/manifest/manifest_group.cc @@ -131,6 +131,238 @@ ManifestGroup::~ManifestGroup() = default; ManifestGroup::ManifestGroup(ManifestGroup&&) noexcept = default; ManifestGroup& ManifestGroup::operator=(ManifestGroup&&) noexcept = default; +class ManifestGroup::FilePlanningIterator final + : public Iterator> { + public: + static Result>>> Make( + std::unique_ptr group) { + ICEBERG_RETURN_UNEXPECTED(group->CheckErrors()); + + group->delete_index_builder_.WithScanMetrics(group->scan_metrics_); + ICEBERG_ASSIGN_OR_RAISE(auto delete_index, + group->delete_index_builder_.Build()); + + const bool drop_stats = ManifestReader::ShouldDropStats(group->columns_); + if (delete_index->has_equality_deletes()) { + group->columns_ = ManifestReader::WithStatsColumns(group->columns_); + } + + std::unique_ptr data_file_evaluator; + if (group->file_filter_ && + group->file_filter_->op() != Expression::Operation::kTrue) { + ICEBERG_ASSIGN_OR_RAISE( + data_file_evaluator, + Evaluator::Make(*DataFileFilterSchema(), group->file_filter_, + group->case_sensitive_)); + } + + return std::unique_ptr>>( + new FilePlanningIterator(std::move(group), std::move(delete_index), + std::move(data_file_evaluator), drop_stats)); + } + + Result>> NextImpl() override { + while (true) { + if (!entry_iterator_) { + ICEBERG_ASSIGN_OR_RAISE(bool opened, OpenNextManifest()); + if (!opened) { + return std::nullopt; + } + } + + ICEBERG_ASSIGN_OR_RAISE(auto entry, entry_iterator_->Next()); + if (!entry.has_value()) { + entry_iterator_.reset(); + continue; + } + + auto value = std::move(entry).value(); + if (group_->ignore_existing_ && + value.status == ManifestStatus::kExisting) { + IncrementSkippedDataFiles(); + continue; + } + + ICEBERG_DCHECK(value.data_file != nullptr, "Data file cannot be null"); + if (data_file_evaluator_) { + DataFileStructLike data_file(*value.data_file); + ICEBERG_ASSIGN_OR_RAISE(bool should_match, + data_file_evaluator_->Evaluate(data_file)); + if (!should_match) { + IncrementSkippedDataFiles(); + continue; + } + } + + if (!group_->manifest_entry_predicate_(value)) { + IncrementSkippedDataFiles(); + continue; + } + + if (drop_stats_) { + ContentFileUtil::DropAllStats(*value.data_file); + } else if (!group_->columns_to_keep_stats_.empty()) { + ContentFileUtil::DropUnselectedStats(*value.data_file, + group_->columns_to_keep_stats_); + } + + ICEBERG_ASSIGN_OR_RAISE(auto delete_files, delete_index_->ForEntry(value)); + UpdateResultMetrics(*value.data_file, delete_files); + + ICEBERG_ASSIGN_OR_RAISE(auto residuals, + GetResidualEvaluator(current_spec_id_)); + ICEBERG_ASSIGN_OR_RAISE( + auto residual, + residuals->ResidualFor(value.data_file->partition)); + + return std::optional>{ + std::make_shared(std::move(value.data_file), + std::move(delete_files), + std::move(residual))}; + } + } + + private: + FilePlanningIterator(std::unique_ptr group, + std::unique_ptr delete_index, + std::unique_ptr data_file_evaluator, + bool drop_stats) + : group_(std::move(group)), + delete_index_(std::move(delete_index)), + data_file_evaluator_(std::move(data_file_evaluator)), + drop_stats_(drop_stats) {} + + Result GetManifestEvaluator(int32_t spec_id) { + auto cached = manifest_evaluators_.find(spec_id); + if (cached != manifest_evaluators_.end()) { + return cached->second.get(); + } + + auto spec_iter = group_->specs_by_id_.find(spec_id); + ICEBERG_CHECK(spec_iter != group_->specs_by_id_.cend(), + "Cannot find partition spec for ID {}", spec_id); + + const auto& spec = spec_iter->second; + auto projector = + Projections::Inclusive(*spec, *group_->schema_, group_->case_sensitive_); + ICEBERG_ASSIGN_OR_RAISE(auto partition_filter, + projector->Project(group_->data_filter_)); + ICEBERG_ASSIGN_OR_RAISE( + partition_filter, + And::Make(std::move(partition_filter), group_->partition_filter_)); + ICEBERG_ASSIGN_OR_RAISE( + auto evaluator, + ManifestEvaluator::MakePartitionFilter(std::move(partition_filter), spec, + *group_->schema_, + group_->case_sensitive_)); + auto* result = evaluator.get(); + manifest_evaluators_.emplace(spec_id, std::move(evaluator)); + return result; + } + + Result GetResidualEvaluator(int32_t spec_id) { + auto cached = residual_evaluators_.find(spec_id); + if (cached != residual_evaluators_.end()) { + return cached->second.get(); + } + + auto spec_iter = group_->specs_by_id_.find(spec_id); + ICEBERG_CHECK(spec_iter != group_->specs_by_id_.cend(), + "Cannot find partition spec for ID {}", spec_id); + + ICEBERG_ASSIGN_OR_RAISE( + auto evaluator, + ResidualEvaluator::Make( + (group_->ignore_residuals_ ? True::Instance() : group_->data_filter_), + *spec_iter->second, *group_->schema_, group_->case_sensitive_)); + auto* result = evaluator.get(); + residual_evaluators_.emplace(spec_id, std::move(evaluator)); + return result; + } + + Result OpenNextManifest() { + while (next_manifest_ < group_->data_manifests_.size()) { + const auto& manifest = group_->data_manifests_[next_manifest_++]; + const int32_t spec_id = manifest.partition_spec_id; + + ICEBERG_ASSIGN_OR_RAISE(auto evaluator, + GetManifestEvaluator(spec_id)); + ICEBERG_ASSIGN_OR_RAISE(bool should_match, + evaluator->Evaluate(manifest)); + if (!should_match) { + IncrementSkippedDataManifests(); + continue; + } + if (group_->ignore_deleted_ && !manifest.has_added_files() && + !manifest.has_existing_files()) { + IncrementSkippedDataManifests(); + continue; + } + if (group_->ignore_existing_ && !manifest.has_added_files() && + !manifest.has_deleted_files()) { + IncrementSkippedDataManifests(); + continue; + } + + if (group_->scan_metrics_) { + group_->scan_metrics_->scanned_data_manifests->Increment(1); + } + + ICEBERG_ASSIGN_OR_RAISE(auto reader, group_->MakeReader(manifest)); + ICEBERG_ASSIGN_OR_RAISE( + entry_iterator_, + group_->ignore_deleted_ ? reader->LiveEntriesIterator() + : reader->EntriesIterator()); + current_spec_id_ = spec_id; + return true; + } + return false; + } + + void IncrementSkippedDataManifests() { + if (group_->scan_metrics_) { + group_->scan_metrics_->skipped_data_manifests->Increment(1); + } + } + + void IncrementSkippedDataFiles() { + if (group_->scan_metrics_) { + group_->scan_metrics_->skipped_data_files->Increment(1); + } + } + + void UpdateResultMetrics( + const DataFile& data_file, + const std::vector>& delete_files) { + if (!group_->scan_metrics_) { + return; + } + + group_->scan_metrics_->total_file_size_in_bytes->Increment( + ContentFileUtil::ContentSizeInBytes(data_file)); + group_->scan_metrics_->result_data_files->Increment(1); + group_->scan_metrics_->result_delete_files->Increment( + static_cast(delete_files.size())); + int64_t deletes_size = 0; + for (const auto& delete_file : delete_files) { + deletes_size += ContentFileUtil::ContentSizeInBytes(*delete_file); + } + group_->scan_metrics_->total_delete_file_size_in_bytes->Increment(deletes_size); + } + + std::unique_ptr group_; + std::unique_ptr delete_index_; + std::unique_ptr data_file_evaluator_; + std::unordered_map> + manifest_evaluators_; + std::unordered_map> + residual_evaluators_; + std::unique_ptr> entry_iterator_; + size_t next_manifest_ = 0; + int32_t current_spec_id_ = 0; + bool drop_stats_; +}; + ManifestGroup& ManifestGroup::FilterData(std::shared_ptr filter) { ICEBERG_BUILDER_ASSIGN_OR_RETURN(data_filter_, And::Make(data_filter_, filter)); delete_index_builder_.DataFilter(std::move(filter)); @@ -251,6 +483,12 @@ Result>> ManifestGroup::PlanFiles() { return file_tasks; } +Result>>> +ManifestGroup::PlanFilesIterator() { + auto group = std::make_unique(std::move(*this)); + return FilePlanningIterator::Make(std::move(group)); +} + Result>> ManifestGroup::Plan( const CreateTasksFunction& create_tasks) { std::unordered_map> residual_cache; diff --git a/src/iceberg/manifest/manifest_group.h b/src/iceberg/manifest/manifest_group.h index be0ca4b8b..74d4c76b9 100644 --- a/src/iceberg/manifest/manifest_group.h +++ b/src/iceberg/manifest/manifest_group.h @@ -37,6 +37,7 @@ #include "iceberg/type_fwd.h" #include "iceberg/util/error_collector.h" #include "iceberg/util/executor.h" +#include "iceberg/util/iterator.h" namespace iceberg { @@ -136,6 +137,16 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector { /// \brief Plan scan tasks for all matching data files. Result>> PlanFiles(); + /// \brief Lazily plan scan tasks for matching data files. + /// + /// The returned iterator owns the planning state and may outlive this ManifestGroup. + /// It reads one manifest batch at a time instead of materializing all manifest entries + /// and scan tasks. Creating the iterator consumes this group's configuration. Streaming + /// planning is pull-based and does not eagerly submit manifests to the executor set by + /// PlanWith(). + Result>>> + PlanFilesIterator(); + /// \brief Get all matching manifest entries. Result> Entries(); @@ -151,6 +162,8 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector { const CreateTasksFunction& create_tasks); private: + class FilePlanningIterator; + ManifestGroup(std::shared_ptr io, std::shared_ptr schema, std::unordered_map> specs_by_id, std::vector data_manifests, diff --git a/src/iceberg/manifest/manifest_reader.cc b/src/iceberg/manifest/manifest_reader.cc index 8da5befcb..88b0c76b9 100644 --- a/src/iceberg/manifest/manifest_reader.cc +++ b/src/iceberg/manifest/manifest_reader.cc @@ -25,6 +25,7 @@ #include #include #include +#include #include @@ -689,8 +690,143 @@ Result> ProjectSchema(std::shared_ptr schema, return schema; } +template +class VectorIterator final : public Iterator { + public: + explicit VectorIterator(std::vector values) : values_(std::move(values)) {} + + Result> NextImpl() override { + if (next_ == values_.size()) { + return std::nullopt; + } + return std::optional{std::move(values_[next_++])}; + } + + private: + std::vector values_; + size_t next_ = 0; +}; + +class ManifestEntryIteratorImpl final : public Iterator { + public: + ManifestEntryIteratorImpl( + std::unique_ptr reader, std::shared_ptr file_schema, + ArrowSchema arrow_schema, + std::shared_ptr inheritable_metadata, + std::optional first_row_id, bool is_committed, bool only_live, + std::unique_ptr evaluator, + std::unique_ptr metrics_evaluator, + std::shared_ptr partition_set, std::shared_ptr skip_counter, + bool drop_stats) + : reader_(std::move(reader)), + file_schema_(std::move(file_schema)), + arrow_schema_(std::exchange(arrow_schema, ArrowSchema{})), + arrow_schema_guard_(&arrow_schema_), + inheritable_metadata_(std::move(inheritable_metadata)), + first_row_id_(first_row_id), + is_committed_(is_committed), + only_live_(only_live), + evaluator_(std::move(evaluator)), + metrics_evaluator_(std::move(metrics_evaluator)), + partition_set_(std::move(partition_set)), + skip_counter_(std::move(skip_counter)), + drop_stats_(drop_stats) {} + + Result> NextImpl() override { + while (true) { + while (next_entry_ < entries_.size()) { + auto entry = std::move(entries_[next_entry_++]); + ICEBERG_RETURN_UNEXPECTED(inheritable_metadata_->Apply(entry)); + + if (only_live_ && !entry.IsAlive()) { + continue; + } + + ICEBERG_DCHECK(entry.data_file != nullptr, "Data file cannot be null"); + if (evaluator_) { + ICEBERG_ASSIGN_OR_RAISE( + bool partition_match, + evaluator_->Evaluate(entry.data_file->partition)); + if (!partition_match) { + IncrementSkipCounter(); + continue; + } + } + if (metrics_evaluator_) { + ICEBERG_ASSIGN_OR_RAISE(bool metrics_match, + metrics_evaluator_->Evaluate(*entry.data_file)); + if (!metrics_match) { + IncrementSkipCounter(); + continue; + } + } + if (partition_set_) { + ICEBERG_PRECHECK(entry.data_file->partition_spec_id.has_value(), + "Missing partition spec id from data file {}", + entry.data_file->file_path); + if (!partition_set_->contains(entry.data_file->partition_spec_id.value(), + entry.data_file->partition)) { + IncrementSkipCounter(); + continue; + } + } + + if (drop_stats_) { + ContentFileUtil::DropAllStats(*entry.data_file); + } + return std::optional{std::move(entry)}; + } + + entries_.clear(); + next_entry_ = 0; + ICEBERG_ASSIGN_OR_RAISE(auto batch, reader_->Next()); + if (!batch.has_value()) { + return std::nullopt; + } + + internal::ArrowArrayGuard array_guard(&batch.value()); + ICEBERG_ASSIGN_OR_RAISE( + entries_, ParseManifestEntry(&arrow_schema_, &batch.value(), *file_schema_, + first_row_id_, is_committed_)); + } + } + + private: + void IncrementSkipCounter() { + if (skip_counter_) { + skip_counter_->Increment(1); + } + } + + std::unique_ptr reader_; + std::shared_ptr file_schema_; + ArrowSchema arrow_schema_{}; + internal::ArrowSchemaGuard arrow_schema_guard_; + std::shared_ptr inheritable_metadata_; + std::optional first_row_id_; + bool is_committed_; + bool only_live_; + std::unique_ptr evaluator_; + std::unique_ptr metrics_evaluator_; + std::shared_ptr partition_set_; + std::shared_ptr skip_counter_; + bool drop_stats_; + std::vector entries_; + size_t next_entry_ = 0; +}; + } // namespace +Result>> ManifestReader::EntriesIterator() { + ICEBERG_ASSIGN_OR_RAISE(auto entries, Entries()); + return std::make_unique>(std::move(entries)); +} + +Result>> ManifestReader::LiveEntriesIterator() { + ICEBERG_ASSIGN_OR_RAISE(auto entries, LiveEntries()); + return std::make_unique>(std::move(entries)); +} + bool ManifestReader::ShouldDropStats(const std::vector& columns) { // Make sure we only drop all stats if we had projected all stats. // We do not drop stats even if we had partially added some stats columns, except for @@ -861,14 +997,25 @@ Status ManifestReaderImpl::OpenReader(std::shared_ptr projection) { } Result> ManifestReaderImpl::Entries() { - return ReadEntries(/*only_live=*/false); + ICEBERG_ASSIGN_OR_RAISE(auto entries, EntriesIterator()); + return entries->ToVector(); } Result> ManifestReaderImpl::LiveEntries() { - return ReadEntries(/*only_live=*/true); + ICEBERG_ASSIGN_OR_RAISE(auto entries, LiveEntriesIterator()); + return entries->ToVector(); +} + +Result>> ManifestReaderImpl::EntriesIterator() { + return MakeEntriesIterator(/*only_live=*/false); +} + +Result>> ManifestReaderImpl::LiveEntriesIterator() { + return MakeEntriesIterator(/*only_live=*/true); } -Result> ManifestReaderImpl::ReadEntries(bool only_live) { +Result>> +ManifestReaderImpl::MakeEntriesIterator(bool only_live) { ICEBERG_ASSIGN_OR_RAISE(auto partition_type, spec_->RawPartitionType(*schema_)); auto data_file_schema = DataFile::Type(std::move(partition_type))->ToSchema(); @@ -894,74 +1041,29 @@ Result> ManifestReaderImpl::ReadEntries(bool only_liv ICEBERG_RETURN_UNEXPECTED(OpenReader(std::move(projected_data_file_schema))); ICEBERG_DCHECK(file_reader_ != nullptr, "File reader should be initialized"); - std::vector manifest_entries; ICEBERG_ASSIGN_OR_RAISE(auto arrow_schema, file_reader_->Schema()); internal::ArrowSchemaGuard schema_guard(&arrow_schema); // Get evaluators if needed - Evaluator* evaluator = nullptr; - InclusiveMetricsEvaluator* metrics_evaluator = nullptr; + std::unique_ptr evaluator; + std::unique_ptr metrics_evaluator; if (HasPartitionFilter() || HasRowFilter()) { - ICEBERG_ASSIGN_OR_RAISE(evaluator, GetEvaluator()); + ICEBERG_ASSIGN_OR_RAISE(std::ignore, GetEvaluator()); + evaluator = std::move(evaluator_); } if (HasRowFilter()) { - ICEBERG_ASSIGN_OR_RAISE(metrics_evaluator, GetMetricsEvaluator()); + ICEBERG_ASSIGN_OR_RAISE(std::ignore, GetMetricsEvaluator()); + metrics_evaluator = std::move(metrics_evaluator_); } bool drop_stats = drop_stats_ && ShouldDropStats(columns_); - - while (true) { - ICEBERG_ASSIGN_OR_RAISE(auto result, file_reader_->Next()); - if (!result.has_value()) { - break; // EOF - } - - internal::ArrowArrayGuard array_guard(&result.value()); - ICEBERG_ASSIGN_OR_RAISE( - auto entries, ParseManifestEntry(&arrow_schema, &result.value(), *file_schema_, - first_row_id_, is_committed_)); - - for (auto& entry : entries) { - ICEBERG_RETURN_UNEXPECTED(inheritable_metadata_->Apply(entry)); - - if (only_live && !entry.IsAlive()) { - continue; - } - - if (needs_filtering) { - ICEBERG_DCHECK(entry.data_file != nullptr, "Data file cannot be null"); - if (evaluator) { - ICEBERG_ASSIGN_OR_RAISE(bool partition_match, - evaluator->Evaluate(entry.data_file->partition)); - if (!partition_match) { - if (skip_counter_) skip_counter_->Increment(1); - continue; - } - } - if (metrics_evaluator) { - ICEBERG_ASSIGN_OR_RAISE(bool metrics_match, - metrics_evaluator->Evaluate(*entry.data_file)); - if (!metrics_match) { - if (skip_counter_) skip_counter_->Increment(1); - continue; - } - } - ICEBERG_ASSIGN_OR_RAISE(bool in_partition_set, InPartitionSet(*entry.data_file)); - if (!in_partition_set) { - if (skip_counter_) skip_counter_->Increment(1); - continue; - } - } - - if (drop_stats) { - ContentFileUtil::DropAllStats(*entry.data_file); - } - - manifest_entries.push_back(std::move(entry)); - } - } - - return manifest_entries; + auto iterator = std::unique_ptr>(new ManifestEntryIteratorImpl( + std::move(file_reader_), file_schema_, std::move(arrow_schema), + inheritable_metadata_, first_row_id_, is_committed_, only_live, + std::move(evaluator), std::move(metrics_evaluator), partition_set_, skip_counter_, + drop_stats)); + schema_guard.Release(); + return iterator; } Result> ManifestListReaderImpl::Files() const { diff --git a/src/iceberg/manifest/manifest_reader.h b/src/iceberg/manifest/manifest_reader.h index 72cb9ae56..7a441e78e 100644 --- a/src/iceberg/manifest/manifest_reader.h +++ b/src/iceberg/manifest/manifest_reader.h @@ -33,6 +33,7 @@ #include "iceberg/metrics/counter.h" #include "iceberg/result.h" #include "iceberg/type_fwd.h" +#include "iceberg/util/iterator.h" namespace iceberg { @@ -42,13 +43,24 @@ class ICEBERG_EXPORT ManifestReader { virtual ~ManifestReader() = default; /// \brief Read all manifest entries in the manifest file. - /// - /// TODO(gangwu): provide a lazy-evaluated iterator interface for better performance. virtual Result> Entries() = 0; /// \brief Read only live (non-deleted) manifest entries. virtual Result> LiveEntries() = 0; + /// \brief Lazily read manifest entries. + /// + /// The returned iterator reads and filters one underlying record batch at a time. This + /// bounds memory use for large manifests. The iterator owns its reader resources and may + /// outlive this ManifestReader. + virtual Result>> EntriesIterator(); + + /// \brief Lazily read only live (non-deleted) manifest entries. + /// + /// The default implementation adapts LiveEntries() for compatibility with custom reader + /// implementations. Built-in readers override this with a streaming implementation. + virtual Result>> LiveEntriesIterator(); + /// \brief Select specific columns of data file to read from the manifest entries. /// /// \note Column names should match the names in `DataFile` schema. Unmatched names diff --git a/src/iceberg/manifest/manifest_reader_internal.h b/src/iceberg/manifest/manifest_reader_internal.h index 4ad708e43..da2335484 100644 --- a/src/iceberg/manifest/manifest_reader_internal.h +++ b/src/iceberg/manifest/manifest_reader_internal.h @@ -66,6 +66,10 @@ class ManifestReaderImpl : public ManifestReader { Result> LiveEntries() override; + Result>> EntriesIterator() override; + + Result>> LiveEntriesIterator() override; + ManifestReader& Select(const std::vector& columns) override; ManifestReader& FilterPartitions(std::shared_ptr expr) override; @@ -81,8 +85,8 @@ class ManifestReaderImpl : public ManifestReader { ManifestReader& SkipCounter(std::shared_ptr counter) override; private: - /// \brief Read entries with optional live-only filtering. - Result> ReadEntries(bool only_live); + /// \brief Create an entry iterator with optional live-only filtering. + Result>> MakeEntriesIterator(bool only_live); /// \brief Lazily open the underlying Avro reader with appropriate schema projection. Status OpenReader(std::shared_ptr projection); @@ -108,7 +112,7 @@ class ManifestReaderImpl : public ManifestReader { const std::shared_ptr file_io_; const std::shared_ptr schema_; const std::shared_ptr spec_; - const std::unique_ptr inheritable_metadata_; + const std::shared_ptr inheritable_metadata_; std::optional first_row_id_; bool is_committed_; diff --git a/src/iceberg/table_scan.cc b/src/iceberg/table_scan.cc index ef4e94c5b..ecda1dfad 100644 --- a/src/iceberg/table_scan.cc +++ b/src/iceberg/table_scan.cc @@ -20,6 +20,7 @@ #include "iceberg/table_scan.h" #include +#include #include #include @@ -64,6 +65,95 @@ const std::vector kScanColumnsWithStats = [] { return cols; }(); +template +class EmptyIterator final : public Iterator { + public: + Result> NextImpl() override { return std::nullopt; } +}; + +Result MakeScanReport(const DataTableScan& scan, const Snapshot& snapshot, + ScanMetricsResult scan_metrics) { + ICEBERG_ASSIGN_OR_RAISE(auto schema_ptr, scan.schema()); + + ICEBERG_ASSIGN_OR_RAISE( + auto projected_id_set, + GetProjectedIdsVisitor::GetProjectedIds(*schema_ptr, /*include_struct_ids=*/true)); + std::vector projected_field_ids(projected_id_set.begin(), + projected_id_set.end()); + std::ranges::sort(projected_field_ids); + + std::vector projected_field_names; + projected_field_names.reserve(projected_field_ids.size()); + for (int32_t field_id : projected_field_ids) { + ICEBERG_ASSIGN_OR_RAISE(auto field_name, schema_ptr->FindColumnNameById(field_id)); + ICEBERG_CHECK(field_name.has_value(), "Projected field {} not found in schema", + field_id); + projected_field_names.emplace_back(*field_name); + } + + ICEBERG_ASSIGN_OR_RAISE( + auto sanitized_filter, + SanitizeExpression::Sanitize(*schema_ptr, scan.filter(), + scan.context().case_sensitive)); + + return ScanReport{ + .table_name = scan.context().table_name, + .snapshot_id = snapshot.snapshot_id, + .filter = std::move(sanitized_filter), + .schema_id = schema_ptr->schema_id(), + .projected_field_ids = std::move(projected_field_ids), + .projected_field_names = std::move(projected_field_names), + .scan_metrics = std::move(scan_metrics), + .metadata = scan.context().options, + }; +} + +class ReportingFileTaskIterator final + : public Iterator> { + public: + ReportingFileTaskIterator( + std::unique_ptr>> iterator, + std::shared_ptr scan_metrics, + std::chrono::nanoseconds planning_duration, + std::shared_ptr reporter, ScanReport report) + : iterator_(std::move(iterator)), + scan_metrics_(std::move(scan_metrics)), + planning_duration_(std::move(planning_duration)), + reporter_(std::move(reporter)), + report_(std::move(report)) {} + + ~ReportingFileTaskIterator() override { Finalize(); } + + Result>> NextImpl() override { + auto start = std::chrono::steady_clock::now(); + auto result = iterator_->Next(); + planning_duration_ += std::chrono::duration_cast( + std::chrono::steady_clock::now() - start); + if (result.has_value() && !result.value().has_value()) { + Finalize(); + } + return result; + } + + private: + void Finalize() { + if (finalized_) { + return; + } + finalized_ = true; + scan_metrics_->total_planning_duration->Record(planning_duration_); + report_.scan_metrics = scan_metrics_->ToResult(); + std::ignore = reporter_->Report(report_); + } + + std::unique_ptr>> iterator_; + std::shared_ptr scan_metrics_; + std::chrono::nanoseconds planning_duration_; + std::shared_ptr reporter_; + ScanReport report_; + bool finalized_ = false; +}; + } // namespace namespace internal { @@ -572,39 +662,8 @@ Status DataTableScan::ReportScan(const Snapshot& snapshot, return {}; } - ICEBERG_ASSIGN_OR_RAISE(auto projected_schema, ResolveProjectedSchema()); - const auto& schema_ptr = projected_schema.get(); - - ICEBERG_ASSIGN_OR_RAISE( - auto projected_id_set, - GetProjectedIdsVisitor::GetProjectedIds(*schema_ptr, /*include_struct_ids=*/true)); - std::vector projected_field_ids(projected_id_set.begin(), - projected_id_set.end()); - std::ranges::sort(projected_field_ids); - - std::vector projected_field_names; - projected_field_names.reserve(projected_field_ids.size()); - for (int32_t field_id : projected_field_ids) { - ICEBERG_ASSIGN_OR_RAISE(auto field_name, schema_ptr->FindColumnNameById(field_id)); - ICEBERG_CHECK(field_name.has_value(), "Projected field {} not found in schema", - field_id); - projected_field_names.emplace_back(*field_name); - } - - ICEBERG_ASSIGN_OR_RAISE( - auto sanitized_filter, - SanitizeExpression::Sanitize(*schema_ptr, filter(), context_.case_sensitive)); - - ScanReport report{ - .table_name = context_.table_name, - .snapshot_id = snapshot.snapshot_id, - .filter = std::move(sanitized_filter), - .schema_id = schema_ptr->schema_id(), - .projected_field_ids = std::move(projected_field_ids), - .projected_field_names = std::move(projected_field_names), - .scan_metrics = scan_metrics.ToResult(), - .metadata = context_.options, - }; + ICEBERG_ASSIGN_OR_RAISE(auto report, + MakeScanReport(*this, snapshot, scan_metrics.ToResult())); return context_.metrics_reporter->Report(report); } @@ -661,6 +720,72 @@ Result>> DataTableScan::PlanFiles() co return tasks; } +Result>>> +DataTableScan::PlanFilesIterator() const { + ICEBERG_ASSIGN_OR_RAISE(auto snapshot, this->snapshot()); + if (!snapshot) { + return std::make_unique>>(); + } + + std::shared_ptr scan_metrics; + std::optional planning_start; + if (context_.metrics_reporter) { + auto metrics_context = MetricsContext::Default(); + scan_metrics = ScanMetrics::Make(*metrics_context); + planning_start = std::chrono::steady_clock::now(); + } + + TableMetadataCache metadata_cache(metadata_.get()); + ICEBERG_ASSIGN_OR_RAISE(auto specs_by_id, metadata_cache.GetPartitionSpecsById()); + + SnapshotCache snapshot_cache(snapshot.get()); + ICEBERG_ASSIGN_OR_RAISE(auto data_manifests, snapshot_cache.DataManifests(io_)); + ICEBERG_ASSIGN_OR_RAISE(auto delete_manifests, snapshot_cache.DeleteManifests(io_)); + + if (scan_metrics) { + scan_metrics->total_data_manifests->Increment( + static_cast(data_manifests.size())); + scan_metrics->total_delete_manifests->Increment( + static_cast(delete_manifests.size())); + } + + ICEBERG_ASSIGN_OR_RAISE( + auto manifest_group, + ManifestGroup::Make(io_, schema_, specs_by_id, + {data_manifests.begin(), data_manifests.end()}, + {delete_manifests.begin(), delete_manifests.end()})); + manifest_group->CaseSensitive(context_.case_sensitive) + .Select(ScanColumns()) + .FilterData(filter()) + .IgnoreDeleted() + .ColumnsToKeepStats(context_.columns_to_keep_stats) + .PlanWith(context_.plan_executor) + .WithScanMetrics(scan_metrics); + if (context_.ignore_residuals) { + manifest_group->IgnoreResiduals(); + } + + ICEBERG_ASSIGN_OR_RAISE(auto iterator, manifest_group->PlanFilesIterator()); + if (!planning_start.has_value()) { + return iterator; + } + + auto planning_duration = std::chrono::duration_cast( + std::chrono::steady_clock::now() - planning_start.value()); + + auto report = MakeScanReport(*this, *snapshot, ScanMetricsResult{}); + if (!report.has_value()) { + // Scan reporting is best effort, matching PlanFiles(). + return iterator; + } + + return std::unique_ptr>>( + new ReportingFileTaskIterator( + std::move(iterator), std::move(scan_metrics), planning_duration, + context_.metrics_reporter, + std::move(report).value())); +} + // Friend function template for IncrementalScan that implements the shared PlanFiles // logic. It resolves the from/to snapshot range from the scan context and delegates // to the two-arg virtual PlanFiles() override in the concrete subclass. diff --git a/src/iceberg/table_scan.h b/src/iceberg/table_scan.h index bee2b7d1d..4c73f39d5 100644 --- a/src/iceberg/table_scan.h +++ b/src/iceberg/table_scan.h @@ -36,6 +36,7 @@ #include "iceberg/type_fwd.h" #include "iceberg/util/error_collector.h" #include "iceberg/util/executor.h" +#include "iceberg/util/iterator.h" namespace iceberg { @@ -463,6 +464,13 @@ class ICEBERG_EXPORT DataTableScan : public TableScan { /// \return A Result containing scan tasks or an error. Result>> PlanFiles() const; + /// \brief Lazily plans scan tasks by resolving manifests and data files on demand. + /// + /// Unlike PlanFiles(), this method does not materialize all manifest entries and scan + /// tasks. The iterator owns its planning resources and can outlive this scan. + Result>>> + PlanFilesIterator() const; + private: Status ReportScan(const Snapshot& snapshot, const ScanMetrics& scan_metrics) const; diff --git a/src/iceberg/test/manifest_reader_test.cc b/src/iceberg/test/manifest_reader_test.cc index b57b0bc4a..8c5ff5314 100644 --- a/src/iceberg/test/manifest_reader_test.cc +++ b/src/iceberg/test/manifest_reader_test.cc @@ -190,6 +190,37 @@ TEST_P(TestManifestReader, TestManifestReaderWithEmptyInheritableMetadata) { EXPECT_EQ(read_entry.snapshot_id, 1000L); } +TEST_P(TestManifestReader, EntriesIteratorOwnsReaderResources) { + auto version = GetParam(); + auto file_a = + MakeDataFile("/path/to/data-a.parquet", PartitionValues({Literal::Int(0)})); + auto file_b = + MakeDataFile("/path/to/data-b.parquet", PartitionValues({Literal::Int(1)})); + + std::vector entries; + entries.push_back( + MakeEntry(ManifestStatus::kAdded, /*snapshot_id=*/1000L, std::move(file_a))); + entries.push_back( + MakeEntry(ManifestStatus::kAdded, /*snapshot_id=*/1000L, std::move(file_b))); + auto manifest = WriteManifest(version, /*snapshot_id=*/1000L, entries); + + ICEBERG_UNWRAP_OR_FAIL(auto reader, + ManifestReader::Make(manifest, file_io_, schema_, spec_)); + ICEBERG_UNWRAP_OR_FAIL(auto iterator, reader->EntriesIterator()); + reader.reset(); + + ICEBERG_UNWRAP_OR_FAIL(auto first, iterator->Next()); + ASSERT_TRUE(first.has_value()); + EXPECT_EQ(first->data_file->file_path, "/path/to/data-a.parquet"); + + ICEBERG_UNWRAP_OR_FAIL(auto second, iterator->Next()); + ASSERT_TRUE(second.has_value()); + EXPECT_EQ(second->data_file->file_path, "/path/to/data-b.parquet"); + + ICEBERG_UNWRAP_OR_FAIL(auto end, iterator->Next()); + EXPECT_FALSE(end.has_value()); +} + TEST_P(TestManifestReader, DeletedEntriesDoNotInheritFirstRowId) { auto version = GetParam(); if (version < 3) { diff --git a/src/iceberg/test/table_scan_test.cc b/src/iceberg/test/table_scan_test.cc index 375c9aa53..186959fd8 100644 --- a/src/iceberg/test/table_scan_test.cc +++ b/src/iceberg/test/table_scan_test.cc @@ -320,6 +320,10 @@ TEST_P(TableScanTest, DataTableScanPlanFilesEmpty) { ICEBERG_UNWRAP_OR_FAIL(auto scan, builder->Build()); ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); EXPECT_TRUE(tasks.empty()); + + ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesIterator()); + ICEBERG_UNWRAP_OR_FAIL(auto next, iterator->Next()); + EXPECT_FALSE(next.has_value()); } TEST_P(TableScanTest, PlanFilesWithDataManifests) { @@ -380,6 +384,14 @@ TEST_P(TableScanTest, PlanFilesWithDataManifests) { ASSERT_EQ(tasks.size(), 2); EXPECT_THAT(GetPaths(tasks), testing::UnorderedElementsAre("/path/to/data1.parquet", "/path/to/data2.parquet")); + + ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesIterator()); + scan.reset(); + ICEBERG_UNWRAP_OR_FAIL(auto streamed_tasks, iterator->ToVector()); + ASSERT_EQ(streamed_tasks.size(), 2); + EXPECT_THAT(GetPaths(streamed_tasks), + testing::UnorderedElementsAre("/path/to/data1.parquet", + "/path/to/data2.parquet")); } TEST_P(TableScanTest, PlanRowLineage) { From f951a1aa05f8109d195a4f9cd218cf0102495002 Mon Sep 17 00:00:00 2001 From: manuzhang Date: Thu, 6 Aug 2026 12:26:32 +0800 Subject: [PATCH 02/21] style: apply clang-format Co-authored-by: Codex --- src/iceberg/manifest/manifest_group.cc | 61 ++++++++++--------------- src/iceberg/manifest/manifest_group.h | 3 +- src/iceberg/manifest/manifest_reader.cc | 29 ++++++------ src/iceberg/manifest/manifest_reader.h | 4 +- src/iceberg/table_scan.cc | 17 +++---- src/iceberg/table_scan.h | 4 +- src/iceberg/test/table_scan_test.cc | 6 +-- 7 files changed, 52 insertions(+), 72 deletions(-) diff --git a/src/iceberg/manifest/manifest_group.cc b/src/iceberg/manifest/manifest_group.cc index 327a19db6..8ac2f2120 100644 --- a/src/iceberg/manifest/manifest_group.cc +++ b/src/iceberg/manifest/manifest_group.cc @@ -139,8 +139,7 @@ class ManifestGroup::FilePlanningIterator final ICEBERG_RETURN_UNEXPECTED(group->CheckErrors()); group->delete_index_builder_.WithScanMetrics(group->scan_metrics_); - ICEBERG_ASSIGN_OR_RAISE(auto delete_index, - group->delete_index_builder_.Build()); + ICEBERG_ASSIGN_OR_RAISE(auto delete_index, group->delete_index_builder_.Build()); const bool drop_stats = ManifestReader::ShouldDropStats(group->columns_); if (delete_index->has_equality_deletes()) { @@ -177,8 +176,7 @@ class ManifestGroup::FilePlanningIterator final } auto value = std::move(entry).value(); - if (group_->ignore_existing_ && - value.status == ManifestStatus::kExisting) { + if (group_->ignore_existing_ && value.status == ManifestStatus::kExisting) { IncrementSkippedDataFiles(); continue; } @@ -209,24 +207,19 @@ class ManifestGroup::FilePlanningIterator final ICEBERG_ASSIGN_OR_RAISE(auto delete_files, delete_index_->ForEntry(value)); UpdateResultMetrics(*value.data_file, delete_files); - ICEBERG_ASSIGN_OR_RAISE(auto residuals, - GetResidualEvaluator(current_spec_id_)); - ICEBERG_ASSIGN_OR_RAISE( - auto residual, - residuals->ResidualFor(value.data_file->partition)); + ICEBERG_ASSIGN_OR_RAISE(auto residuals, GetResidualEvaluator(current_spec_id_)); + ICEBERG_ASSIGN_OR_RAISE(auto residual, + residuals->ResidualFor(value.data_file->partition)); - return std::optional>{ - std::make_shared(std::move(value.data_file), - std::move(delete_files), - std::move(residual))}; + return std::optional>{std::make_shared( + std::move(value.data_file), std::move(delete_files), std::move(residual))}; } } private: FilePlanningIterator(std::unique_ptr group, std::unique_ptr delete_index, - std::unique_ptr data_file_evaluator, - bool drop_stats) + std::unique_ptr data_file_evaluator, bool drop_stats) : group_(std::move(group)), delete_index_(std::move(delete_index)), data_file_evaluator_(std::move(data_file_evaluator)), @@ -247,14 +240,12 @@ class ManifestGroup::FilePlanningIterator final Projections::Inclusive(*spec, *group_->schema_, group_->case_sensitive_); ICEBERG_ASSIGN_OR_RAISE(auto partition_filter, projector->Project(group_->data_filter_)); - ICEBERG_ASSIGN_OR_RAISE( - partition_filter, - And::Make(std::move(partition_filter), group_->partition_filter_)); - ICEBERG_ASSIGN_OR_RAISE( - auto evaluator, - ManifestEvaluator::MakePartitionFilter(std::move(partition_filter), spec, - *group_->schema_, - group_->case_sensitive_)); + ICEBERG_ASSIGN_OR_RAISE(partition_filter, And::Make(std::move(partition_filter), + group_->partition_filter_)); + ICEBERG_ASSIGN_OR_RAISE(auto evaluator, + ManifestEvaluator::MakePartitionFilter( + std::move(partition_filter), spec, *group_->schema_, + group_->case_sensitive_)); auto* result = evaluator.get(); manifest_evaluators_.emplace(spec_id, std::move(evaluator)); return result; @@ -285,10 +276,8 @@ class ManifestGroup::FilePlanningIterator final const auto& manifest = group_->data_manifests_[next_manifest_++]; const int32_t spec_id = manifest.partition_spec_id; - ICEBERG_ASSIGN_OR_RAISE(auto evaluator, - GetManifestEvaluator(spec_id)); - ICEBERG_ASSIGN_OR_RAISE(bool should_match, - evaluator->Evaluate(manifest)); + ICEBERG_ASSIGN_OR_RAISE(auto evaluator, GetManifestEvaluator(spec_id)); + ICEBERG_ASSIGN_OR_RAISE(bool should_match, evaluator->Evaluate(manifest)); if (!should_match) { IncrementSkippedDataManifests(); continue; @@ -309,10 +298,9 @@ class ManifestGroup::FilePlanningIterator final } ICEBERG_ASSIGN_OR_RAISE(auto reader, group_->MakeReader(manifest)); - ICEBERG_ASSIGN_OR_RAISE( - entry_iterator_, - group_->ignore_deleted_ ? reader->LiveEntriesIterator() - : reader->EntriesIterator()); + ICEBERG_ASSIGN_OR_RAISE(entry_iterator_, group_->ignore_deleted_ + ? reader->LiveEntriesIterator() + : reader->EntriesIterator()); current_spec_id_ = spec_id; return true; } @@ -331,9 +319,8 @@ class ManifestGroup::FilePlanningIterator final } } - void UpdateResultMetrics( - const DataFile& data_file, - const std::vector>& delete_files) { + void UpdateResultMetrics(const DataFile& data_file, + const std::vector>& delete_files) { if (!group_->scan_metrics_) { return; } @@ -353,10 +340,8 @@ class ManifestGroup::FilePlanningIterator final std::unique_ptr group_; std::unique_ptr delete_index_; std::unique_ptr data_file_evaluator_; - std::unordered_map> - manifest_evaluators_; - std::unordered_map> - residual_evaluators_; + std::unordered_map> manifest_evaluators_; + std::unordered_map> residual_evaluators_; std::unique_ptr> entry_iterator_; size_t next_manifest_ = 0; int32_t current_spec_id_ = 0; diff --git a/src/iceberg/manifest/manifest_group.h b/src/iceberg/manifest/manifest_group.h index 74d4c76b9..c9778747a 100644 --- a/src/iceberg/manifest/manifest_group.h +++ b/src/iceberg/manifest/manifest_group.h @@ -144,8 +144,7 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector { /// and scan tasks. Creating the iterator consumes this group's configuration. Streaming /// planning is pull-based and does not eagerly submit manifests to the executor set by /// PlanWith(). - Result>>> - PlanFilesIterator(); + Result>>> PlanFilesIterator(); /// \brief Get all matching manifest entries. Result> Entries(); diff --git a/src/iceberg/manifest/manifest_reader.cc b/src/iceberg/manifest/manifest_reader.cc index 88b0c76b9..190f586b2 100644 --- a/src/iceberg/manifest/manifest_reader.cc +++ b/src/iceberg/manifest/manifest_reader.cc @@ -709,15 +709,14 @@ class VectorIterator final : public Iterator { class ManifestEntryIteratorImpl final : public Iterator { public: - ManifestEntryIteratorImpl( - std::unique_ptr reader, std::shared_ptr file_schema, - ArrowSchema arrow_schema, - std::shared_ptr inheritable_metadata, - std::optional first_row_id, bool is_committed, bool only_live, - std::unique_ptr evaluator, - std::unique_ptr metrics_evaluator, - std::shared_ptr partition_set, std::shared_ptr skip_counter, - bool drop_stats) + ManifestEntryIteratorImpl(std::unique_ptr reader, + std::shared_ptr file_schema, ArrowSchema arrow_schema, + std::shared_ptr inheritable_metadata, + std::optional first_row_id, bool is_committed, + bool only_live, std::unique_ptr evaluator, + std::unique_ptr metrics_evaluator, + std::shared_ptr partition_set, + std::shared_ptr skip_counter, bool drop_stats) : reader_(std::move(reader)), file_schema_(std::move(file_schema)), arrow_schema_(std::exchange(arrow_schema, ArrowSchema{})), @@ -744,9 +743,8 @@ class ManifestEntryIteratorImpl final : public Iterator { ICEBERG_DCHECK(entry.data_file != nullptr, "Data file cannot be null"); if (evaluator_) { - ICEBERG_ASSIGN_OR_RAISE( - bool partition_match, - evaluator_->Evaluate(entry.data_file->partition)); + ICEBERG_ASSIGN_OR_RAISE(bool partition_match, + evaluator_->Evaluate(entry.data_file->partition)); if (!partition_match) { IncrementSkipCounter(); continue; @@ -1010,12 +1008,13 @@ Result>> ManifestReaderImpl::EntriesIter return MakeEntriesIterator(/*only_live=*/false); } -Result>> ManifestReaderImpl::LiveEntriesIterator() { +Result>> +ManifestReaderImpl::LiveEntriesIterator() { return MakeEntriesIterator(/*only_live=*/true); } -Result>> -ManifestReaderImpl::MakeEntriesIterator(bool only_live) { +Result>> ManifestReaderImpl::MakeEntriesIterator( + bool only_live) { ICEBERG_ASSIGN_OR_RAISE(auto partition_type, spec_->RawPartitionType(*schema_)); auto data_file_schema = DataFile::Type(std::move(partition_type))->ToSchema(); diff --git a/src/iceberg/manifest/manifest_reader.h b/src/iceberg/manifest/manifest_reader.h index 7a441e78e..a789e112b 100644 --- a/src/iceberg/manifest/manifest_reader.h +++ b/src/iceberg/manifest/manifest_reader.h @@ -51,8 +51,8 @@ class ICEBERG_EXPORT ManifestReader { /// \brief Lazily read manifest entries. /// /// The returned iterator reads and filters one underlying record batch at a time. This - /// bounds memory use for large manifests. The iterator owns its reader resources and may - /// outlive this ManifestReader. + /// bounds memory use for large manifests. The iterator owns its reader resources and + /// may outlive this ManifestReader. virtual Result>> EntriesIterator(); /// \brief Lazily read only live (non-deleted) manifest entries. diff --git a/src/iceberg/table_scan.cc b/src/iceberg/table_scan.cc index ecda1dfad..e989aab11 100644 --- a/src/iceberg/table_scan.cc +++ b/src/iceberg/table_scan.cc @@ -91,10 +91,9 @@ Result MakeScanReport(const DataTableScan& scan, const Snapshot& sna projected_field_names.emplace_back(*field_name); } - ICEBERG_ASSIGN_OR_RAISE( - auto sanitized_filter, - SanitizeExpression::Sanitize(*schema_ptr, scan.filter(), - scan.context().case_sensitive)); + ICEBERG_ASSIGN_OR_RAISE(auto sanitized_filter, + SanitizeExpression::Sanitize(*schema_ptr, scan.filter(), + scan.context().case_sensitive)); return ScanReport{ .table_name = scan.context().table_name, @@ -108,8 +107,7 @@ Result MakeScanReport(const DataTableScan& scan, const Snapshot& sna }; } -class ReportingFileTaskIterator final - : public Iterator> { +class ReportingFileTaskIterator final : public Iterator> { public: ReportingFileTaskIterator( std::unique_ptr>> iterator, @@ -780,10 +778,9 @@ DataTableScan::PlanFilesIterator() const { } return std::unique_ptr>>( - new ReportingFileTaskIterator( - std::move(iterator), std::move(scan_metrics), planning_duration, - context_.metrics_reporter, - std::move(report).value())); + new ReportingFileTaskIterator(std::move(iterator), std::move(scan_metrics), + planning_duration, context_.metrics_reporter, + std::move(report).value())); } // Friend function template for IncrementalScan that implements the shared PlanFiles diff --git a/src/iceberg/table_scan.h b/src/iceberg/table_scan.h index 4c73f39d5..6c99e4c5f 100644 --- a/src/iceberg/table_scan.h +++ b/src/iceberg/table_scan.h @@ -468,8 +468,8 @@ class ICEBERG_EXPORT DataTableScan : public TableScan { /// /// Unlike PlanFiles(), this method does not materialize all manifest entries and scan /// tasks. The iterator owns its planning resources and can outlive this scan. - Result>>> - PlanFilesIterator() const; + Result>>> PlanFilesIterator() + const; private: Status ReportScan(const Snapshot& snapshot, const ScanMetrics& scan_metrics) const; diff --git a/src/iceberg/test/table_scan_test.cc b/src/iceberg/test/table_scan_test.cc index 186959fd8..5a57c9bcf 100644 --- a/src/iceberg/test/table_scan_test.cc +++ b/src/iceberg/test/table_scan_test.cc @@ -389,9 +389,9 @@ TEST_P(TableScanTest, PlanFilesWithDataManifests) { scan.reset(); ICEBERG_UNWRAP_OR_FAIL(auto streamed_tasks, iterator->ToVector()); ASSERT_EQ(streamed_tasks.size(), 2); - EXPECT_THAT(GetPaths(streamed_tasks), - testing::UnorderedElementsAre("/path/to/data1.parquet", - "/path/to/data2.parquet")); + EXPECT_THAT( + GetPaths(streamed_tasks), + testing::UnorderedElementsAre("/path/to/data1.parquet", "/path/to/data2.parquet")); } TEST_P(TableScanTest, PlanRowLineage) { From 36cbec9d95225003e5ce5705160864ff4b0440ad Mon Sep 17 00:00:00 2001 From: manuzhang Date: Thu, 6 Aug 2026 12:54:58 +0800 Subject: [PATCH 03/21] fix: propagate evaluator errors directly Co-authored-by: Codex --- src/iceberg/manifest/manifest_reader.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/iceberg/manifest/manifest_reader.cc b/src/iceberg/manifest/manifest_reader.cc index 190f586b2..59124b774 100644 --- a/src/iceberg/manifest/manifest_reader.cc +++ b/src/iceberg/manifest/manifest_reader.cc @@ -1047,11 +1047,11 @@ Result>> ManifestReaderImpl::MakeEntries std::unique_ptr evaluator; std::unique_ptr metrics_evaluator; if (HasPartitionFilter() || HasRowFilter()) { - ICEBERG_ASSIGN_OR_RAISE(std::ignore, GetEvaluator()); + ICEBERG_RETURN_UNEXPECTED(GetEvaluator()); evaluator = std::move(evaluator_); } if (HasRowFilter()) { - ICEBERG_ASSIGN_OR_RAISE(std::ignore, GetMetricsEvaluator()); + ICEBERG_RETURN_UNEXPECTED(GetMetricsEvaluator()); metrics_evaluator = std::move(metrics_evaluator_); } From 48ad0226b29678409d79eb087ad45e063c34573a Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Mon, 17 Aug 2026 19:05:18 +0800 Subject: [PATCH 04/21] fix: address lazy scan planning review Co-authored-by: Codex --- src/iceberg/manifest/manifest_group.cc | 10 ++++- src/iceberg/manifest/manifest_reader.cc | 15 ++++--- .../manifest/manifest_reader_internal.h | 8 ++-- src/iceberg/test/table_scan_test.cc | 39 +++++++++++++------ 4 files changed, 47 insertions(+), 25 deletions(-) diff --git a/src/iceberg/manifest/manifest_group.cc b/src/iceberg/manifest/manifest_group.cc index 8ac2f2120..660b1045e 100644 --- a/src/iceberg/manifest/manifest_group.cc +++ b/src/iceberg/manifest/manifest_group.cc @@ -197,6 +197,10 @@ class ManifestGroup::FilePlanningIterator final continue; } + ICEBERG_ASSIGN_OR_RAISE(auto delete_files, delete_index_->ForEntry(value)); + + // Equality-delete matching uses data-file statistics. Drop unrequested stats only + // after the delete index has finished matching this entry. if (drop_stats_) { ContentFileUtil::DropAllStats(*value.data_file); } else if (!group_->columns_to_keep_stats_.empty()) { @@ -204,7 +208,6 @@ class ManifestGroup::FilePlanningIterator final group_->columns_to_keep_stats_); } - ICEBERG_ASSIGN_OR_RAISE(auto delete_files, delete_index_->ForEntry(value)); UpdateResultMetrics(*value.data_file, delete_files); ICEBERG_ASSIGN_OR_RAISE(auto residuals, GetResidualEvaluator(current_spec_id_)); @@ -428,12 +431,15 @@ Result>> ManifestGroup::PlanFiles() { tasks.reserve(entries.size()); for (auto& entry : entries) { + ICEBERG_ASSIGN_OR_RAISE(auto delete_files, ctx.deletes->ForEntry(entry)); + + // Equality-delete matching uses data-file statistics. Drop unrequested stats only + // after the delete index has finished matching this entry. if (ctx.drop_stats) { ContentFileUtil::DropAllStats(*entry.data_file); } else if (!ctx.columns_to_keep_stats.empty()) { ContentFileUtil::DropUnselectedStats(*entry.data_file, ctx.columns_to_keep_stats); } - ICEBERG_ASSIGN_OR_RAISE(auto delete_files, ctx.deletes->ForEntry(entry)); // Count result metrics once per data file task. A delete file shared by // multiple data files contributes once to each task, unlike indexed delete files. if (scan_metrics_) { diff --git a/src/iceberg/manifest/manifest_reader.cc b/src/iceberg/manifest/manifest_reader.cc index 59124b774..69be83364 100644 --- a/src/iceberg/manifest/manifest_reader.cc +++ b/src/iceberg/manifest/manifest_reader.cc @@ -922,7 +922,7 @@ bool ManifestReaderImpl::HasRowFilter() const { return row_filter_->op() != Expression::Operation::kTrue; } -Result ManifestReaderImpl::GetEvaluator() { +Result> ManifestReaderImpl::TakeEvaluator() { if (!evaluator_) { auto projection_evaluator = Projections::Inclusive(*spec_, *schema_, case_sensitive_); ICEBERG_ASSIGN_OR_RAISE(auto projected, projection_evaluator->Project(row_filter_)); @@ -935,16 +935,17 @@ Result ManifestReaderImpl::GetEvaluator() { evaluator_, Evaluator::Make(*partition_schema, std::move(final_part_filter), case_sensitive_)); } - return evaluator_.get(); + return std::move(evaluator_); } -Result ManifestReaderImpl::GetMetricsEvaluator() { +Result> +ManifestReaderImpl::TakeMetricsEvaluator() { if (!metrics_evaluator_) { ICEBERG_ASSIGN_OR_RAISE( metrics_evaluator_, InclusiveMetricsEvaluator::Make(row_filter_, *schema_, case_sensitive_)); } - return metrics_evaluator_.get(); + return std::move(metrics_evaluator_); } Result ManifestReaderImpl::InPartitionSet(const DataFile& file) const { @@ -1047,12 +1048,10 @@ Result>> ManifestReaderImpl::MakeEntries std::unique_ptr evaluator; std::unique_ptr metrics_evaluator; if (HasPartitionFilter() || HasRowFilter()) { - ICEBERG_RETURN_UNEXPECTED(GetEvaluator()); - evaluator = std::move(evaluator_); + ICEBERG_ASSIGN_OR_RAISE(evaluator, TakeEvaluator()); } if (HasRowFilter()) { - ICEBERG_RETURN_UNEXPECTED(GetMetricsEvaluator()); - metrics_evaluator = std::move(metrics_evaluator_); + ICEBERG_ASSIGN_OR_RAISE(metrics_evaluator, TakeMetricsEvaluator()); } bool drop_stats = drop_stats_ && ShouldDropStats(columns_); diff --git a/src/iceberg/manifest/manifest_reader_internal.h b/src/iceberg/manifest/manifest_reader_internal.h index da2335484..e1b243f88 100644 --- a/src/iceberg/manifest/manifest_reader_internal.h +++ b/src/iceberg/manifest/manifest_reader_internal.h @@ -97,11 +97,11 @@ class ManifestReaderImpl : public ManifestReader { /// \brief Check if there's a non-trivial row filter. bool HasRowFilter() const; - /// \brief Get or create the partition evaluator. - Result GetEvaluator(); + /// \brief Get or create and transfer ownership of the partition evaluator. + Result> TakeEvaluator(); - /// \brief Get or create the metrics evaluator. - Result GetMetricsEvaluator(); + /// \brief Get or create and transfer ownership of the metrics evaluator. + Result> TakeMetricsEvaluator(); /// \brief Check if a partition is in the partition set. Result InPartitionSet(const DataFile& file) const; diff --git a/src/iceberg/test/table_scan_test.cc b/src/iceberg/test/table_scan_test.cc index 5a57c9bcf..5c07926fc 100644 --- a/src/iceberg/test/table_scan_test.cc +++ b/src/iceberg/test/table_scan_test.cc @@ -616,22 +616,27 @@ TEST_P(TableScanTest, PlanFilesWithDeleteFiles) { std::vector data_entries{ MakeEntry(ManifestStatus::kAdded, kSnapshotId, /*sequence_number=*/1, MakeDataFile("/path/to/data1.parquet", part_value, - partitioned_spec_->spec_id(), /*record_count=*/100)), + partitioned_spec_->spec_id(), /*record_count=*/100, + /*lower_id=*/0, /*upper_id=*/10)), MakeEntry(ManifestStatus::kAdded, kSnapshotId, /*sequence_number=*/1, MakeDataFile("/path/to/data2.parquet", part_value, - partitioned_spec_->spec_id(), /*record_count=*/200))}; + partitioned_spec_->spec_id(), /*record_count=*/200, + /*lower_id=*/20, /*upper_id=*/30))}; auto data_manifest = WriteDataManifest(version, kSnapshotId, std::move(data_entries), partitioned_spec_); // Create delete manifest with position delete files + auto equality_delete = MakeEqualityDeleteFile("/path/to/eq_delete.parquet", part_value, + partitioned_spec_->spec_id(), {1}); + equality_delete->lower_bounds[1] = Literal::Int(20).Serialize().value(); + equality_delete->upper_bounds[1] = Literal::Int(30).Serialize().value(); std::vector delete_entries{ MakeEntry( ManifestStatus::kAdded, kSnapshotId, /*sequence_number=*/2, MakePositionDeleteFile("/path/to/pos_delete.parquet", part_value, partitioned_spec_->spec_id(), "/path/to/data1.parquet")), MakeEntry(ManifestStatus::kAdded, kSnapshotId, /*sequence_number=*/2, - MakeEqualityDeleteFile("/path/to/eq_delete.parquet", part_value, - partitioned_spec_->spec_id(), {1}))}; + std::move(equality_delete))}; auto delete_manifest = WriteDeleteManifest( version, kSnapshotId, std::move(delete_entries), partitioned_spec_); std::string manifest_list_path = WriteManifestList( @@ -674,13 +679,25 @@ TEST_P(TableScanTest, PlanFilesWithDeleteFiles) { MakeScanBuilder(metadata_with_manifests)); ICEBERG_UNWRAP_OR_FAIL(auto scan, builder->Build()); ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); - ASSERT_EQ(tasks.size(), 2); - EXPECT_THAT(GetPaths(tasks), testing::UnorderedElementsAre("/path/to/data1.parquet", - "/path/to/data2.parquet")); - // Verify that delete files are associated with the tasks - for (const auto& task : tasks) { - EXPECT_GT(task->delete_files().size(), 0); - } + auto verify_tasks = [](const auto& planned_tasks) { + ASSERT_EQ(planned_tasks.size(), 2); + for (const auto& task : planned_tasks) { + ASSERT_EQ(task->delete_files().size(), 1); + if (task->data_file()->file_path == "/path/to/data1.parquet") { + EXPECT_EQ(task->delete_files().front()->file_path, "/path/to/pos_delete.parquet"); + } else { + EXPECT_EQ(task->data_file()->file_path, "/path/to/data2.parquet"); + EXPECT_EQ(task->delete_files().front()->file_path, "/path/to/eq_delete.parquet"); + } + EXPECT_TRUE(task->data_file()->lower_bounds.empty()); + EXPECT_TRUE(task->data_file()->upper_bounds.empty()); + } + }; + verify_tasks(tasks); + + ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesIterator()); + ICEBERG_UNWRAP_OR_FAIL(auto streamed_tasks, iterator->ToVector()); + verify_tasks(streamed_tasks); } TEST_P(TableScanTest, SchemaWithSelectedColumnsAndFilter) { From c1c540407dc369092f561c15bd14d09dfd7a0fa2 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Mon, 17 Aug 2026 19:48:54 +0800 Subject: [PATCH 05/21] Preserve ManifestReader ABI for iterators Move lazy iterator virtuals to an optional extension interface and keep non-virtual compatibility helpers on ManifestReader. Add coverage for readers implementing only the original interface. Co-authored-by: Codex --- src/iceberg/manifest/manifest_reader.cc | 6 ++ src/iceberg/manifest/manifest_reader.h | 26 +++++--- .../manifest/manifest_reader_internal.h | 2 +- src/iceberg/test/manifest_reader_test.cc | 60 +++++++++++++++++++ src/iceberg/type_fwd.h | 1 + 5 files changed, 87 insertions(+), 8 deletions(-) diff --git a/src/iceberg/manifest/manifest_reader.cc b/src/iceberg/manifest/manifest_reader.cc index 69be83364..e929460b9 100644 --- a/src/iceberg/manifest/manifest_reader.cc +++ b/src/iceberg/manifest/manifest_reader.cc @@ -816,11 +816,17 @@ class ManifestEntryIteratorImpl final : public Iterator { } // namespace Result>> ManifestReader::EntriesIterator() { + if (auto* iterable = dynamic_cast(this)) { + return iterable->EntriesIterator(); + } ICEBERG_ASSIGN_OR_RAISE(auto entries, Entries()); return std::make_unique>(std::move(entries)); } Result>> ManifestReader::LiveEntriesIterator() { + if (auto* iterable = dynamic_cast(this)) { + return iterable->LiveEntriesIterator(); + } ICEBERG_ASSIGN_OR_RAISE(auto entries, LiveEntries()); return std::make_unique>(std::move(entries)); } diff --git a/src/iceberg/manifest/manifest_reader.h b/src/iceberg/manifest/manifest_reader.h index a789e112b..f0a26d320 100644 --- a/src/iceberg/manifest/manifest_reader.h +++ b/src/iceberg/manifest/manifest_reader.h @@ -50,16 +50,15 @@ class ICEBERG_EXPORT ManifestReader { /// \brief Lazily read manifest entries. /// - /// The returned iterator reads and filters one underlying record batch at a time. This - /// bounds memory use for large manifests. The iterator owns its reader resources and - /// may outlive this ManifestReader. - virtual Result>> EntriesIterator(); + /// Implementations using SupportsManifestEntryIteration stream entries lazily. Other + /// implementations are adapted from Entries() for compatibility. + Result>> EntriesIterator(); /// \brief Lazily read only live (non-deleted) manifest entries. /// - /// The default implementation adapts LiveEntries() for compatibility with custom reader - /// implementations. Built-in readers override this with a streaming implementation. - virtual Result>> LiveEntriesIterator(); + /// Implementations using SupportsManifestEntryIteration stream entries lazily. Other + /// implementations are adapted from LiveEntries() for compatibility. + Result>> LiveEntriesIterator(); /// \brief Select specific columns of data file to read from the manifest entries. /// @@ -146,6 +145,19 @@ class ICEBERG_EXPORT ManifestReader { const std::vector& columns); }; +/// \brief Optional mix-in for ManifestReader implementations that support lazy entry +/// iteration. +class ICEBERG_EXPORT SupportsManifestEntryIteration { + public: + virtual ~SupportsManifestEntryIteration() = default; + + /// \brief Lazily read manifest entries. + virtual Result>> EntriesIterator() = 0; + + /// \brief Lazily read only live (non-deleted) manifest entries. + virtual Result>> LiveEntriesIterator() = 0; +}; + /// \brief Read manifest files from a manifest list file. class ICEBERG_EXPORT ManifestListReader { public: diff --git a/src/iceberg/manifest/manifest_reader_internal.h b/src/iceberg/manifest/manifest_reader_internal.h index e1b243f88..9b9352a24 100644 --- a/src/iceberg/manifest/manifest_reader_internal.h +++ b/src/iceberg/manifest/manifest_reader_internal.h @@ -43,7 +43,7 @@ namespace iceberg { /// This implementation supports lazy reader creation and filtering based on /// partition expressions, row expressions, and partition sets. Following the /// Java implementation pattern. -class ManifestReaderImpl : public ManifestReader { +class ManifestReaderImpl : public ManifestReader, public SupportsManifestEntryIteration { public: /// \brief Construct a ManifestReaderImpl for lazy initialization. /// diff --git a/src/iceberg/test/manifest_reader_test.cc b/src/iceberg/test/manifest_reader_test.cc index 8c5ff5314..65b2926da 100644 --- a/src/iceberg/test/manifest_reader_test.cc +++ b/src/iceberg/test/manifest_reader_test.cc @@ -42,6 +42,66 @@ namespace iceberg { +namespace { + +class LegacyManifestReader : public ManifestReader { + public: + Result> Entries() override { + ++entries_calls; + return std::vector{}; + } + + Result> LiveEntries() override { + ++live_entries_calls; + return std::vector{}; + } + + ManifestReader& Select(const std::vector& /*columns*/) override { + return *this; + } + + ManifestReader& FilterPartitions(std::shared_ptr /*expr*/) override { + return *this; + } + + ManifestReader& FilterPartitions( + std::shared_ptr /*partition_set*/) override { + return *this; + } + + ManifestReader& FilterRows(std::shared_ptr /*expr*/) override { + return *this; + } + + ManifestReader& CaseSensitive(bool /*case_sensitive*/) override { return *this; } + + ManifestReader& TryDropStats() override { return *this; } + + ManifestReader& SkipCounter(std::shared_ptr /*counter*/) override { + return *this; + } + + int entries_calls = 0; + int live_entries_calls = 0; +}; + +TEST(ManifestReaderCompatibilityTest, IteratorHelpersAdaptLegacyReaders) { + LegacyManifestReader reader; + ManifestReader& base_reader = reader; + + ICEBERG_UNWRAP_OR_FAIL(auto entries, base_reader.EntriesIterator()); + ICEBERG_UNWRAP_OR_FAIL(auto entry, entries->Next()); + EXPECT_FALSE(entry.has_value()); + EXPECT_EQ(reader.entries_calls, 1); + + ICEBERG_UNWRAP_OR_FAIL(auto live_entries, base_reader.LiveEntriesIterator()); + ICEBERG_UNWRAP_OR_FAIL(auto live_entry, live_entries->Next()); + EXPECT_FALSE(live_entry.has_value()); + EXPECT_EQ(reader.live_entries_calls, 1); +} + +} // namespace + class TestManifestReader : public testing::TestWithParam { protected: void SetUp() override { diff --git a/src/iceberg/type_fwd.h b/src/iceberg/type_fwd.h index 0b19adaf5..83fcb8051 100644 --- a/src/iceberg/type_fwd.h +++ b/src/iceberg/type_fwd.h @@ -197,6 +197,7 @@ class ManifestListReader; class ManifestListWriter; class ManifestReader; class ManifestWriter; +class SupportsManifestEntryIteration; class PartitionSummary; /// \brief File I/O. From 97189dbed74272e5f979a14c916c2fed781e4674 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Mon, 17 Aug 2026 21:28:38 +0800 Subject: [PATCH 06/21] Preserve select-all projection with equality deletes Keep empty and wildcard select-all sentinels unchanged when equality-delete matching requests statistics. Add coverage for lazy manifest planning with the default projection. Co-authored-by: Codex --- src/iceberg/manifest/manifest_reader.cc | 2 +- src/iceberg/test/manifest_group_test.cc | 45 ++++++++++++++++++++++++ src/iceberg/test/manifest_reader_test.cc | 6 ++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/src/iceberg/manifest/manifest_reader.cc b/src/iceberg/manifest/manifest_reader.cc index e929460b9..edba94a7c 100644 --- a/src/iceberg/manifest/manifest_reader.cc +++ b/src/iceberg/manifest/manifest_reader.cc @@ -856,7 +856,7 @@ bool ManifestReader::ShouldDropStats(const std::vector& columns) { std::vector ManifestReader::WithStatsColumns( const std::vector& columns) { - if (std::ranges::contains(columns, Schema::kAllColumns)) { + if (columns.empty() || std::ranges::contains(columns, Schema::kAllColumns)) { return columns; } else { std::vector updated_columns{columns}; diff --git a/src/iceberg/test/manifest_group_test.cc b/src/iceberg/test/manifest_group_test.cc index aa2d6810d..e147950aa 100644 --- a/src/iceberg/test/manifest_group_test.cc +++ b/src/iceberg/test/manifest_group_test.cc @@ -292,6 +292,51 @@ TEST_P(ManifestGroupTest, CreateAndGetEntries) { EXPECT_EQ(tasks[1]->delete_files()[0]->file_path, "/path/to/delete.parquet"); } +TEST_P(ManifestGroupTest, PlanFilesIteratorPreservesSelectAllWithEqualityDeletes) { + auto version = GetParam(); + if (version < 2) { + GTEST_SKIP() << "Delete files only supported in V2+"; + } + + constexpr int64_t kSnapshotId = 1000L; + const auto part_value = PartitionValues({Literal::Int(0)}); + + auto data_file = MakeDataFile("/path/to/data.parquet", part_value, + partitioned_spec_->spec_id(), /*record_count=*/100); + data_file->lower_bounds[1] = Literal::Int(20).Serialize().value(); + data_file->upper_bounds[1] = Literal::Int(30).Serialize().value(); + std::vector data_entries{MakeEntry( + ManifestStatus::kAdded, kSnapshotId, /*sequence_number=*/1, std::move(data_file))}; + auto data_manifest = + WriteDataManifest(version, kSnapshotId, std::move(data_entries), partitioned_spec_); + + auto equality_delete = MakeEqualityDeleteFile("/path/to/equality-delete.parquet", + part_value, partitioned_spec_->spec_id()); + equality_delete->lower_bounds[1] = Literal::Int(20).Serialize().value(); + equality_delete->upper_bounds[1] = Literal::Int(30).Serialize().value(); + std::vector delete_entries{MakeEntry(ManifestStatus::kAdded, kSnapshotId, + /*sequence_number=*/2, + std::move(equality_delete))}; + auto delete_manifest = WriteDeleteManifest( + version, kSnapshotId, std::move(delete_entries), partitioned_spec_); + + ICEBERG_UNWRAP_OR_FAIL( + auto group, ManifestGroup::Make(file_io_, schema_, GetSpecsById(), {data_manifest}, + {delete_manifest})); + ICEBERG_UNWRAP_OR_FAIL(auto iterator, group->PlanFilesIterator()); + ICEBERG_UNWRAP_OR_FAIL(auto task, iterator->Next()); + + ASSERT_TRUE(task.has_value()); + EXPECT_EQ(task.value()->data_file()->file_path, "/path/to/data.parquet"); + EXPECT_EQ(task.value()->data_file()->record_count, 100); + ASSERT_EQ(task.value()->delete_files().size(), 1); + EXPECT_EQ(task.value()->delete_files().front()->file_path, + "/path/to/equality-delete.parquet"); + + ICEBERG_UNWRAP_OR_FAIL(auto end, iterator->Next()); + EXPECT_FALSE(end.has_value()); +} + TEST_P(ManifestGroupTest, IgnoreDeleted) { auto version = GetParam(); diff --git a/src/iceberg/test/manifest_reader_test.cc b/src/iceberg/test/manifest_reader_test.cc index 65b2926da..af96ca971 100644 --- a/src/iceberg/test/manifest_reader_test.cc +++ b/src/iceberg/test/manifest_reader_test.cc @@ -639,6 +639,12 @@ TEST(ManifestReaderStaticTest, TestShouldDropStats) { ManifestReader::ShouldDropStats({"file_path", "record_count", "value_counts"})); } +TEST(ManifestReaderStaticTest, WithStatsColumnsPreservesSelectAll) { + EXPECT_TRUE(ManifestReader::WithStatsColumns({}).empty()); + EXPECT_THAT(ManifestReader::WithStatsColumns({std::string(Schema::kAllColumns)}), + testing::ElementsAre(Schema::kAllColumns)); +} + INSTANTIATE_TEST_SUITE_P(ManifestReaderVersions, TestManifestReader, testing::Values(1, 2, 3)); From a8608bc038b61f6a4965f0abeeb7dccd411e5fde Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Wed, 2 Sep 2026 18:59:59 +0800 Subject: [PATCH 07/21] fix: address lazy scan iterator review feedback --- src/iceberg/manifest/manifest_group.cc | 135 +++++++++++++----- src/iceberg/manifest/manifest_group.h | 10 +- src/iceberg/manifest/manifest_reader.h | 6 + src/iceberg/table_scan.cc | 14 +- src/iceberg/test/manifest_group_test.cc | 37 ++++- .../test/scan_planning_metrics_test.cc | 69 +++++++++ 6 files changed, 225 insertions(+), 46 deletions(-) diff --git a/src/iceberg/manifest/manifest_group.cc b/src/iceberg/manifest/manifest_group.cc index 660b1045e..4d35a4efc 100644 --- a/src/iceberg/manifest/manifest_group.cc +++ b/src/iceberg/manifest/manifest_group.cc @@ -162,20 +162,12 @@ class ManifestGroup::FilePlanningIterator final Result>> NextImpl() override { while (true) { - if (!entry_iterator_) { - ICEBERG_ASSIGN_OR_RAISE(bool opened, OpenNextManifest()); - if (!opened) { - return std::nullopt; - } - } - - ICEBERG_ASSIGN_OR_RAISE(auto entry, entry_iterator_->Next()); + ICEBERG_ASSIGN_OR_RAISE(auto entry, NextEntry()); if (!entry.has_value()) { - entry_iterator_.reset(); - continue; + return std::nullopt; } - auto value = std::move(entry).value(); + auto [spec_id, value] = std::move(entry).value(); if (group_->ignore_existing_ && value.status == ManifestStatus::kExisting) { IncrementSkippedDataFiles(); continue; @@ -210,7 +202,7 @@ class ManifestGroup::FilePlanningIterator final UpdateResultMetrics(*value.data_file, delete_files); - ICEBERG_ASSIGN_OR_RAISE(auto residuals, GetResidualEvaluator(current_spec_id_)); + ICEBERG_ASSIGN_OR_RAISE(auto residuals, GetResidualEvaluator(spec_id)); ICEBERG_ASSIGN_OR_RAISE(auto residual, residuals->ResidualFor(value.data_file->partition)); @@ -228,6 +220,37 @@ class ManifestGroup::FilePlanningIterator final data_file_evaluator_(std::move(data_file_evaluator)), drop_stats_(drop_stats) {} + using TaggedEntry = std::pair; + + Result> NextEntry() { + if (!group_->executor_.has_value()) { + while (true) { + if (!entry_iterator_) { + ICEBERG_ASSIGN_OR_RAISE(bool opened, OpenNextManifest()); + if (!opened) { + return std::nullopt; + } + } + + ICEBERG_ASSIGN_OR_RAISE(auto entry, entry_iterator_->Next()); + if (!entry.has_value()) { + entry_iterator_.reset(); + continue; + } + return std::optional{std::in_place, current_spec_id_, + std::move(entry).value()}; + } + } + + while (next_batch_entry_ == batch_entries_.size()) { + ICEBERG_ASSIGN_OR_RAISE(bool loaded, LoadNextManifestBatch()); + if (!loaded) { + return std::nullopt; + } + } + return std::optional{std::move(batch_entries_[next_batch_entry_++])}; + } + Result GetManifestEvaluator(int32_t spec_id) { auto cached = manifest_evaluators_.find(spec_id); if (cached != manifest_evaluators_.end()) { @@ -274,42 +297,82 @@ class ManifestGroup::FilePlanningIterator final return result; } + Result ShouldReadManifest(const ManifestFile& manifest) { + ICEBERG_ASSIGN_OR_RAISE(auto evaluator, + GetManifestEvaluator(manifest.partition_spec_id)); + ICEBERG_ASSIGN_OR_RAISE(bool should_match, evaluator->Evaluate(manifest)); + if (!should_match || + (group_->ignore_deleted_ && !manifest.has_added_files() && + !manifest.has_existing_files()) || + (group_->ignore_existing_ && !manifest.has_added_files() && + !manifest.has_deleted_files())) { + IncrementSkippedDataManifests(); + return false; + } + + if (group_->scan_metrics_) { + group_->scan_metrics_->scanned_data_manifests->Increment(1); + } + return true; + } + Result OpenNextManifest() { while (next_manifest_ < group_->data_manifests_.size()) { const auto& manifest = group_->data_manifests_[next_manifest_++]; - const int32_t spec_id = manifest.partition_spec_id; - - ICEBERG_ASSIGN_OR_RAISE(auto evaluator, GetManifestEvaluator(spec_id)); - ICEBERG_ASSIGN_OR_RAISE(bool should_match, evaluator->Evaluate(manifest)); - if (!should_match) { - IncrementSkippedDataManifests(); - continue; - } - if (group_->ignore_deleted_ && !manifest.has_added_files() && - !manifest.has_existing_files()) { - IncrementSkippedDataManifests(); - continue; - } - if (group_->ignore_existing_ && !manifest.has_added_files() && - !manifest.has_deleted_files()) { - IncrementSkippedDataManifests(); + ICEBERG_ASSIGN_OR_RAISE(bool should_read, ShouldReadManifest(manifest)); + if (!should_read) { continue; } - if (group_->scan_metrics_) { - group_->scan_metrics_->scanned_data_manifests->Increment(1); - } - ICEBERG_ASSIGN_OR_RAISE(auto reader, group_->MakeReader(manifest)); ICEBERG_ASSIGN_OR_RAISE(entry_iterator_, group_->ignore_deleted_ ? reader->LiveEntriesIterator() : reader->EntriesIterator()); - current_spec_id_ = spec_id; + current_spec_id_ = manifest.partition_spec_id; return true; } return false; } + Result LoadNextManifestBatch() { + std::vector manifests; + manifests.reserve(kManifestReadBatchSize); + while (next_manifest_ < group_->data_manifests_.size() && + manifests.size() < kManifestReadBatchSize) { + const auto& manifest = group_->data_manifests_[next_manifest_++]; + ICEBERG_ASSIGN_OR_RAISE(bool should_read, ShouldReadManifest(manifest)); + if (should_read) { + manifests.push_back(&manifest); + } + } + + if (manifests.empty()) { + return false; + } + + ICEBERG_ASSIGN_OR_RAISE( + batch_entries_, + ParallelCollect( + group_->executor_, manifests, + [this](const ManifestFile* manifest) -> Result> { + ICEBERG_ASSIGN_OR_RAISE(auto reader, group_->MakeReader(*manifest)); + ICEBERG_ASSIGN_OR_RAISE(auto iterator, group_->ignore_deleted_ + ? reader->LiveEntriesIterator() + : reader->EntriesIterator()); + ICEBERG_ASSIGN_OR_RAISE(auto entries, iterator->ToVector()); + + std::vector tagged_entries; + tagged_entries.reserve(entries.size()); + for (auto& entry : entries) { + tagged_entries.emplace_back(manifest->partition_spec_id, + std::move(entry)); + } + return tagged_entries; + })); + next_batch_entry_ = 0; + return true; + } + void IncrementSkippedDataManifests() { if (group_->scan_metrics_) { group_->scan_metrics_->skipped_data_manifests->Increment(1); @@ -346,9 +409,13 @@ class ManifestGroup::FilePlanningIterator final std::unordered_map> manifest_evaluators_; std::unordered_map> residual_evaluators_; std::unique_ptr> entry_iterator_; + std::vector batch_entries_; size_t next_manifest_ = 0; + size_t next_batch_entry_ = 0; int32_t current_spec_id_ = 0; bool drop_stats_; + + static constexpr size_t kManifestReadBatchSize = 32; }; ManifestGroup& ManifestGroup::FilterData(std::shared_ptr filter) { @@ -475,7 +542,7 @@ Result>> ManifestGroup::PlanFiles() { } Result>>> -ManifestGroup::PlanFilesIterator() { +ManifestGroup::PlanFilesIterator() && { auto group = std::make_unique(std::move(*this)); return FilePlanningIterator::Make(std::move(group)); } diff --git a/src/iceberg/manifest/manifest_group.h b/src/iceberg/manifest/manifest_group.h index c9778747a..2f92fe62a 100644 --- a/src/iceberg/manifest/manifest_group.h +++ b/src/iceberg/manifest/manifest_group.h @@ -140,11 +140,11 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector { /// \brief Lazily plan scan tasks for matching data files. /// /// The returned iterator owns the planning state and may outlive this ManifestGroup. - /// It reads one manifest batch at a time instead of materializing all manifest entries - /// and scan tasks. Creating the iterator consumes this group's configuration. Streaming - /// planning is pull-based and does not eagerly submit manifests to the executor set by - /// PlanWith(). - Result>>> PlanFilesIterator(); + /// It reads one bounded manifest batch at a time instead of materializing all manifest + /// entries and scan tasks. When PlanWith() configures an executor, manifests in each + /// batch are read in parallel. Creating the iterator consumes this group's + /// configuration, so this method may only be called on an rvalue. + Result>>> PlanFilesIterator() &&; /// \brief Get all matching manifest entries. Result> Entries(); diff --git a/src/iceberg/manifest/manifest_reader.h b/src/iceberg/manifest/manifest_reader.h index f0a26d320..163c482b0 100644 --- a/src/iceberg/manifest/manifest_reader.h +++ b/src/iceberg/manifest/manifest_reader.h @@ -152,9 +152,15 @@ class ICEBERG_EXPORT SupportsManifestEntryIteration { virtual ~SupportsManifestEntryIteration() = default; /// \brief Lazily read manifest entries. + /// + /// The returned iterator must own all resources required for iteration and must not + /// depend on this reader remaining alive. virtual Result>> EntriesIterator() = 0; /// \brief Lazily read only live (non-deleted) manifest entries. + /// + /// The returned iterator must own all resources required for iteration and must not + /// depend on this reader remaining alive. virtual Result>> LiveEntriesIterator() = 0; }; diff --git a/src/iceberg/table_scan.cc b/src/iceberg/table_scan.cc index e989aab11..3bda861b8 100644 --- a/src/iceberg/table_scan.cc +++ b/src/iceberg/table_scan.cc @@ -127,7 +127,10 @@ class ReportingFileTaskIterator final : public IteratorNext(); planning_duration_ += std::chrono::duration_cast( std::chrono::steady_clock::now() - start); - if (result.has_value() && !result.value().has_value()) { + if (!result.has_value()) { + // Match PlanFiles(): failed planning does not emit a successful scan report. + finalized_ = true; + } else if (!result.value().has_value()) { Finalize(); } return result; @@ -763,7 +766,7 @@ DataTableScan::PlanFilesIterator() const { manifest_group->IgnoreResiduals(); } - ICEBERG_ASSIGN_OR_RAISE(auto iterator, manifest_group->PlanFilesIterator()); + ICEBERG_ASSIGN_OR_RAISE(auto iterator, std::move(*manifest_group).PlanFilesIterator()); if (!planning_start.has_value()) { return iterator; } @@ -777,10 +780,9 @@ DataTableScan::PlanFilesIterator() const { return iterator; } - return std::unique_ptr>>( - new ReportingFileTaskIterator(std::move(iterator), std::move(scan_metrics), - planning_duration, context_.metrics_reporter, - std::move(report).value())); + return std::make_unique( + std::move(iterator), std::move(scan_metrics), planning_duration, + context_.metrics_reporter, std::move(report).value()); } // Friend function template for IncrementalScan that implements the shared PlanFiles diff --git a/src/iceberg/test/manifest_group_test.cc b/src/iceberg/test/manifest_group_test.cc index e147950aa..0067d6806 100644 --- a/src/iceberg/test/manifest_group_test.cc +++ b/src/iceberg/test/manifest_group_test.cc @@ -323,7 +323,7 @@ TEST_P(ManifestGroupTest, PlanFilesIteratorPreservesSelectAllWithEqualityDeletes ICEBERG_UNWRAP_OR_FAIL( auto group, ManifestGroup::Make(file_io_, schema_, GetSpecsById(), {data_manifest}, {delete_manifest})); - ICEBERG_UNWRAP_OR_FAIL(auto iterator, group->PlanFilesIterator()); + ICEBERG_UNWRAP_OR_FAIL(auto iterator, std::move(*group).PlanFilesIterator()); ICEBERG_UNWRAP_OR_FAIL(auto task, iterator->Next()); ASSERT_TRUE(task.has_value()); @@ -682,6 +682,41 @@ TEST_P(ManifestGroupTest, MultipleDataManifests) { EXPECT_EQ(executor.submit_count(), 2); } +TEST_P(ManifestGroupTest, PlanFilesIteratorUsesExecutor) { + auto version = GetParam(); + + const auto partition_a = PartitionValues({Literal::Int(0)}); + const auto partition_b = PartitionValues({Literal::Int(1)}); + auto data_manifest_1 = + WriteDataManifest(version, /*snapshot_id=*/1000L, + {MakeEntry(ManifestStatus::kAdded, /*snapshot_id=*/1000L, + /*sequence_number=*/1, + MakeDataFile("/path/to/data1.parquet", partition_a, + partitioned_spec_->spec_id()))}, + partitioned_spec_); + auto data_manifest_2 = + WriteDataManifest(version, /*snapshot_id=*/1001L, + {MakeEntry(ManifestStatus::kAdded, /*snapshot_id=*/1001L, + /*sequence_number=*/2, + MakeDataFile("/path/to/data2.parquet", partition_b, + partitioned_spec_->spec_id()))}, + partitioned_spec_); + + ICEBERG_UNWRAP_OR_FAIL( + auto group, + ManifestGroup::Make(file_io_, schema_, GetSpecsById(), + {std::move(data_manifest_1), std::move(data_manifest_2)})); + test::ThreadExecutor executor; + group->PlanWith(std::ref(executor)); + + ICEBERG_UNWRAP_OR_FAIL(auto iterator, std::move(*group).PlanFilesIterator()); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, iterator->ToVector()); + + EXPECT_THAT(GetPaths(tasks), testing::UnorderedElementsAre("/path/to/data1.parquet", + "/path/to/data2.parquet")); + EXPECT_EQ(executor.submit_count(), 2); +} + TEST_P(ManifestGroupTest, PartitionFilter) { auto version = GetParam(); diff --git a/src/iceberg/test/scan_planning_metrics_test.cc b/src/iceberg/test/scan_planning_metrics_test.cc index d3ef4fae9..38ea8796f 100644 --- a/src/iceberg/test/scan_planning_metrics_test.cc +++ b/src/iceberg/test/scan_planning_metrics_test.cc @@ -48,6 +48,7 @@ namespace { class CapturingReporter final : public MetricsReporter { public: Status Report(const MetricsReport& report) override { + ++report_count_; if (std::holds_alternative(report)) { last_ = std::get(report); } @@ -55,9 +56,11 @@ class CapturingReporter final : public MetricsReporter { } const std::optional& last() const { return last_; } + int report_count() const { return report_count_; } private: std::optional last_; + int report_count_ = 0; }; } // namespace @@ -241,6 +244,72 @@ TEST_P(ScanPlanningMetricsTest, ReportsToTableAndScanReporters) { EXPECT_EQ(scan_reporter->last()->table_name, "test.table"); } +TEST_P(ScanPlanningMetricsTest, IteratorReportsWhenDestroyedEarly) { + auto version = GetParam(); + constexpr int64_t kSnapshotId = 2010L; + const auto part = PartitionValues({Literal::Int(0)}); + + auto data_manifest = WriteDataManifest( + version, kSnapshotId, + {MakeEntry( + ManifestStatus::kAdded, kSnapshotId, /*sequence_number=*/1, + MakeDataFile("/data/file_a.parquet", part, partitioned_spec_->spec_id())), + MakeEntry( + ManifestStatus::kAdded, kSnapshotId, /*sequence_number=*/1, + MakeDataFile("/data/file_b.parquet", part, partitioned_spec_->spec_id()))}, + partitioned_spec_); + auto manifest_list = + WriteManifestList(version, kSnapshotId, /*sequence_number=*/1, {data_manifest}); + auto metadata = + BuildMetadata(version, kSnapshotId, /*sequence_number=*/1, manifest_list); + + ICEBERG_UNWRAP_OR_FAIL(auto builder, MakeScanBuilder(metadata)); + builder->ReportWith(reporter_); + ICEBERG_UNWRAP_OR_FAIL(auto scan, builder->Build()); + ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesIterator()); + scan.reset(); + + ICEBERG_UNWRAP_OR_FAIL(auto first, iterator->Next()); + ASSERT_TRUE(first.has_value()); + EXPECT_EQ(reporter_->report_count(), 0); + + iterator.reset(); + ASSERT_EQ(reporter_->report_count(), 1); + ASSERT_TRUE(reporter_->last().has_value()); + const auto& metrics = reporter_->last()->scan_metrics; + ASSERT_TRUE(metrics.result_data_files.has_value()); + EXPECT_EQ(metrics.result_data_files->value, 1); +} + +TEST_P(ScanPlanningMetricsTest, IteratorDoesNotReportFailedPlanning) { + auto version = GetParam(); + constexpr int64_t kSnapshotId = 2011L; + const auto part = PartitionValues({Literal::Int(0)}); + + auto missing_manifest = WriteDataManifest( + version, kSnapshotId, + {MakeEntry(ManifestStatus::kAdded, kSnapshotId, /*sequence_number=*/1, + MakeDataFile("/data/file.parquet", part, partitioned_spec_->spec_id()))}, + partitioned_spec_); + missing_manifest.manifest_path = "missing-data-manifest.avro"; + auto manifest_list = + WriteManifestList(version, kSnapshotId, /*sequence_number=*/1, {missing_manifest}); + auto metadata = + BuildMetadata(version, kSnapshotId, /*sequence_number=*/1, manifest_list); + + ICEBERG_UNWRAP_OR_FAIL(auto builder, MakeScanBuilder(metadata)); + builder->ReportWith(reporter_); + ICEBERG_UNWRAP_OR_FAIL(auto scan, builder->Build()); + ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesIterator()); + + auto next = iterator->Next(); + EXPECT_FALSE(next.has_value()); + EXPECT_EQ(reporter_->report_count(), 0); + + iterator.reset(); + EXPECT_EQ(reporter_->report_count(), 0); +} + TEST_P(ScanPlanningMetricsTest, ScanReportFilterUsesBoundCaseInsensitiveResolution) { auto version = GetParam(); constexpr int64_t kSnapshotId = 2000L; From 12b00b2c9977a4e6682ff3d64e52b849d6226840 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Wed, 2 Sep 2026 22:30:27 +0800 Subject: [PATCH 08/21] fix: stream executor-backed manifest planning Co-authored-by: Codex --- src/iceberg/manifest/manifest_group.cc | 48 ++++++++++++++++---------- src/iceberg/manifest/manifest_group.h | 7 ++-- 2 files changed, 33 insertions(+), 22 deletions(-) diff --git a/src/iceberg/manifest/manifest_group.cc b/src/iceberg/manifest/manifest_group.cc index 4d35a4efc..0661e7ec4 100644 --- a/src/iceberg/manifest/manifest_group.cc +++ b/src/iceberg/manifest/manifest_group.cc @@ -221,6 +221,7 @@ class ManifestGroup::FilePlanningIterator final drop_stats_(drop_stats) {} using TaggedEntry = std::pair; + using TaggedIterator = std::pair>>; Result> NextEntry() { if (!group_->executor_.has_value()) { @@ -242,13 +243,23 @@ class ManifestGroup::FilePlanningIterator final } } - while (next_batch_entry_ == batch_entries_.size()) { - ICEBERG_ASSIGN_OR_RAISE(bool loaded, LoadNextManifestBatch()); - if (!loaded) { - return std::nullopt; + while (true) { + if (next_batch_iterator_ == batch_iterators_.size()) { + ICEBERG_ASSIGN_OR_RAISE(bool loaded, LoadNextManifestBatch()); + if (!loaded) { + return std::nullopt; + } + } + + auto& [spec_id, iterator] = batch_iterators_[next_batch_iterator_]; + ICEBERG_ASSIGN_OR_RAISE(auto entry, iterator->Next()); + if (!entry.has_value()) { + iterator.reset(); + ++next_batch_iterator_; + continue; } + return std::optional{std::in_place, spec_id, std::move(entry).value()}; } - return std::optional{std::move(batch_entries_[next_batch_entry_++])}; } Result GetManifestEvaluator(int32_t spec_id) { @@ -350,26 +361,25 @@ class ManifestGroup::FilePlanningIterator final return false; } + // Open the readers concurrently, but keep their iterators instead of collecting + // entries here. This preserves bounded memory for large manifests while retaining + // parallel manifest initialization when an executor is configured. ICEBERG_ASSIGN_OR_RAISE( - batch_entries_, + batch_iterators_, ParallelCollect( group_->executor_, manifests, - [this](const ManifestFile* manifest) -> Result> { + [this](const ManifestFile* manifest) -> Result> { ICEBERG_ASSIGN_OR_RAISE(auto reader, group_->MakeReader(*manifest)); ICEBERG_ASSIGN_OR_RAISE(auto iterator, group_->ignore_deleted_ ? reader->LiveEntriesIterator() : reader->EntriesIterator()); - ICEBERG_ASSIGN_OR_RAISE(auto entries, iterator->ToVector()); - - std::vector tagged_entries; - tagged_entries.reserve(entries.size()); - for (auto& entry : entries) { - tagged_entries.emplace_back(manifest->partition_spec_id, - std::move(entry)); - } - return tagged_entries; + + std::vector tagged_iterators; + tagged_iterators.emplace_back(manifest->partition_spec_id, + std::move(iterator)); + return tagged_iterators; })); - next_batch_entry_ = 0; + next_batch_iterator_ = 0; return true; } @@ -409,9 +419,9 @@ class ManifestGroup::FilePlanningIterator final std::unordered_map> manifest_evaluators_; std::unordered_map> residual_evaluators_; std::unique_ptr> entry_iterator_; - std::vector batch_entries_; + std::vector batch_iterators_; size_t next_manifest_ = 0; - size_t next_batch_entry_ = 0; + size_t next_batch_iterator_ = 0; int32_t current_spec_id_ = 0; bool drop_stats_; diff --git a/src/iceberg/manifest/manifest_group.h b/src/iceberg/manifest/manifest_group.h index 2f92fe62a..9ad9fa156 100644 --- a/src/iceberg/manifest/manifest_group.h +++ b/src/iceberg/manifest/manifest_group.h @@ -141,9 +141,10 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector { /// /// The returned iterator owns the planning state and may outlive this ManifestGroup. /// It reads one bounded manifest batch at a time instead of materializing all manifest - /// entries and scan tasks. When PlanWith() configures an executor, manifests in each - /// batch are read in parallel. Creating the iterator consumes this group's - /// configuration, so this method may only be called on an rvalue. + /// entries and scan tasks. When PlanWith() configures an executor, entry iterators for + /// manifests in each batch are opened in parallel, while entries are consumed one + /// manifest at a time. Creating the iterator consumes this group's configuration, so + /// this method may only be called on an rvalue. Result>>> PlanFilesIterator() &&; /// \brief Get all matching manifest entries. From 075f2c1bb4ff962d766d484614e4c8731817166e Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Wed, 2 Sep 2026 23:18:27 +0800 Subject: [PATCH 09/21] fix: support copy-only vector iterators Co-authored-by: Codex --- src/iceberg/manifest/manifest_reader.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/iceberg/manifest/manifest_reader.cc b/src/iceberg/manifest/manifest_reader.cc index edba94a7c..d9e5a0de7 100644 --- a/src/iceberg/manifest/manifest_reader.cc +++ b/src/iceberg/manifest/manifest_reader.cc @@ -699,7 +699,7 @@ class VectorIterator final : public Iterator { if (next_ == values_.size()) { return std::nullopt; } - return std::optional{std::move(values_[next_++])}; + return std::optional{std::move_if_noexcept(values_[next_++])}; } private: From fd496ae1f355451bf4786b1aa32423c52e7baa0e Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Thu, 3 Sep 2026 00:02:52 +0800 Subject: [PATCH 10/21] fix: align manifest planning stats semantics Co-authored-by: Codex --- src/iceberg/manifest/manifest_group.cc | 27 +++++++++++++++++-------- src/iceberg/manifest/manifest_group.h | 2 ++ src/iceberg/test/manifest_group_test.cc | 2 ++ 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/iceberg/manifest/manifest_group.cc b/src/iceberg/manifest/manifest_group.cc index 0661e7ec4..c9e6890d9 100644 --- a/src/iceberg/manifest/manifest_group.cc +++ b/src/iceberg/manifest/manifest_group.cc @@ -141,10 +141,8 @@ class ManifestGroup::FilePlanningIterator final group->delete_index_builder_.WithScanMetrics(group->scan_metrics_); ICEBERG_ASSIGN_OR_RAISE(auto delete_index, group->delete_index_builder_.Build()); - const bool drop_stats = ManifestReader::ShouldDropStats(group->columns_); - if (delete_index->has_equality_deletes()) { - group->columns_ = ManifestReader::WithStatsColumns(group->columns_); - } + const bool drop_stats = + group->PrepareStatsProjection(delete_index->has_equality_deletes()); std::unique_ptr data_file_evaluator; if (group->file_filter_ && @@ -425,6 +423,10 @@ class ManifestGroup::FilePlanningIterator final int32_t current_spec_id_ = 0; bool drop_stats_; + // Limit the number of manifest readers and iterators retained by executor-backed + // planning. The executor still controls actual task concurrency, while this fixed + // cap prevents resource use from scaling with the total manifest count. Entries + // within each manifest remain streamed, so this does not cap manifest size. static constexpr size_t kManifestReadBatchSize = 32; }; @@ -582,10 +584,7 @@ Result>> ManifestGroup::Plan( delete_index_builder_.WithScanMetrics(scan_metrics_); ICEBERG_ASSIGN_OR_RAISE(auto delete_index, delete_index_builder_.Build()); - bool drop_stats = ManifestReader::ShouldDropStats(columns_); - if (delete_index->has_equality_deletes()) { - columns_ = ManifestReader::WithStatsColumns(columns_); - } + const bool drop_stats = PrepareStatsProjection(delete_index->has_equality_deletes()); std::unordered_map> task_context_cache; auto get_task_context = [&](int32_t spec_id) -> Result { @@ -679,6 +678,18 @@ Result> ManifestGroup::MakeReader( return reader; } +bool ManifestGroup::PrepareStatsProjection(bool has_equality_deletes) { + // The caller's projection records whether stats were requested. Equality-delete + // matching may add stats temporarily, but they should still be dropped from the + // result when the original projection did not request them. Keeping this decision + // here ensures eager and iterator planning use identical semantics. + const bool drop_stats = ManifestReader::ShouldDropStats(columns_); + if (has_equality_deletes) { + columns_ = ManifestReader::WithStatsColumns(columns_); + } + return drop_stats; +} + Result>> ManifestGroup::ReadEntries() { const auto cache_capacity = static_cast(specs_by_id_.size()); diff --git a/src/iceberg/manifest/manifest_group.h b/src/iceberg/manifest/manifest_group.h index 9ad9fa156..16760942b 100644 --- a/src/iceberg/manifest/manifest_group.h +++ b/src/iceberg/manifest/manifest_group.h @@ -173,6 +173,8 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector { Result> MakeReader(const ManifestFile& manifest); + bool PrepareStatsProjection(bool has_equality_deletes); + std::shared_ptr io_; std::shared_ptr schema_; std::unordered_map> specs_by_id_; diff --git a/src/iceberg/test/manifest_group_test.cc b/src/iceberg/test/manifest_group_test.cc index 0067d6806..797ca7b19 100644 --- a/src/iceberg/test/manifest_group_test.cc +++ b/src/iceberg/test/manifest_group_test.cc @@ -329,6 +329,8 @@ TEST_P(ManifestGroupTest, PlanFilesIteratorPreservesSelectAllWithEqualityDeletes ASSERT_TRUE(task.has_value()); EXPECT_EQ(task.value()->data_file()->file_path, "/path/to/data.parquet"); EXPECT_EQ(task.value()->data_file()->record_count, 100); + EXPECT_TRUE(task.value()->data_file()->lower_bounds.contains(1)); + EXPECT_TRUE(task.value()->data_file()->upper_bounds.contains(1)); ASSERT_EQ(task.value()->delete_files().size(), 1); EXPECT_EQ(task.value()->delete_files().front()->file_path, "/path/to/equality-delete.parquet"); From f7b57f6b54b76aff0090fc403593c3cccb5f8916 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Thu, 3 Sep 2026 12:53:04 +0800 Subject: [PATCH 11/21] refactor: simplify lazy file planning --- src/iceberg/manifest/manifest_group.cc | 20 +++++++++++--------- src/iceberg/manifest/manifest_group.h | 2 +- src/iceberg/table_scan.cc | 7 +++---- src/iceberg/table_scan.h | 3 +-- src/iceberg/type_fwd.h | 4 ++++ 5 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/iceberg/manifest/manifest_group.cc b/src/iceberg/manifest/manifest_group.cc index c9e6890d9..ef0475573 100644 --- a/src/iceberg/manifest/manifest_group.cc +++ b/src/iceberg/manifest/manifest_group.cc @@ -134,7 +134,7 @@ ManifestGroup& ManifestGroup::operator=(ManifestGroup&&) noexcept = default; class ManifestGroup::FilePlanningIterator final : public Iterator> { public: - static Result>>> Make( + static Result Make( std::unique_ptr group) { ICEBERG_RETURN_UNEXPECTED(group->CheckErrors()); @@ -153,7 +153,7 @@ class ManifestGroup::FilePlanningIterator final group->case_sensitive_)); } - return std::unique_ptr>>( + return FileScanTaskIterator( new FilePlanningIterator(std::move(group), std::move(delete_index), std::move(data_file_evaluator), drop_stats)); } @@ -310,11 +310,14 @@ class ManifestGroup::FilePlanningIterator final ICEBERG_ASSIGN_OR_RAISE(auto evaluator, GetManifestEvaluator(manifest.partition_spec_id)); ICEBERG_ASSIGN_OR_RAISE(bool should_match, evaluator->Evaluate(manifest)); - if (!should_match || - (group_->ignore_deleted_ && !manifest.has_added_files() && - !manifest.has_existing_files()) || - (group_->ignore_existing_ && !manifest.has_added_files() && - !manifest.has_deleted_files())) { + const bool has_non_deleted_files = + manifest.has_added_files() || manifest.has_existing_files(); + const bool has_non_existing_files = + manifest.has_added_files() || manifest.has_deleted_files(); + const bool has_only_ignored_files = + (group_->ignore_deleted_ && !has_non_deleted_files) || + (group_->ignore_existing_ && !has_non_existing_files); + if (!should_match || has_only_ignored_files) { IncrementSkippedDataManifests(); return false; } @@ -553,8 +556,7 @@ Result>> ManifestGroup::PlanFiles() { return file_tasks; } -Result>>> -ManifestGroup::PlanFilesIterator() && { +Result ManifestGroup::PlanFilesIterator() && { auto group = std::make_unique(std::move(*this)); return FilePlanningIterator::Make(std::move(group)); } diff --git a/src/iceberg/manifest/manifest_group.h b/src/iceberg/manifest/manifest_group.h index 16760942b..107e684e1 100644 --- a/src/iceberg/manifest/manifest_group.h +++ b/src/iceberg/manifest/manifest_group.h @@ -145,7 +145,7 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector { /// manifests in each batch are opened in parallel, while entries are consumed one /// manifest at a time. Creating the iterator consumes this group's configuration, so /// this method may only be called on an rvalue. - Result>>> PlanFilesIterator() &&; + Result PlanFilesIterator() &&; /// \brief Get all matching manifest entries. Result> Entries(); diff --git a/src/iceberg/table_scan.cc b/src/iceberg/table_scan.cc index 3bda861b8..5448f3050 100644 --- a/src/iceberg/table_scan.cc +++ b/src/iceberg/table_scan.cc @@ -110,7 +110,7 @@ Result MakeScanReport(const DataTableScan& scan, const Snapshot& sna class ReportingFileTaskIterator final : public Iterator> { public: ReportingFileTaskIterator( - std::unique_ptr>> iterator, + FileScanTaskIterator iterator, std::shared_ptr scan_metrics, std::chrono::nanoseconds planning_duration, std::shared_ptr reporter, ScanReport report) @@ -147,7 +147,7 @@ class ReportingFileTaskIterator final : public IteratorReport(report_); } - std::unique_ptr>> iterator_; + FileScanTaskIterator iterator_; std::shared_ptr scan_metrics_; std::chrono::nanoseconds planning_duration_; std::shared_ptr reporter_; @@ -721,8 +721,7 @@ Result>> DataTableScan::PlanFiles() co return tasks; } -Result>>> -DataTableScan::PlanFilesIterator() const { +Result DataTableScan::PlanFilesIterator() const { ICEBERG_ASSIGN_OR_RAISE(auto snapshot, this->snapshot()); if (!snapshot) { return std::make_unique>>(); diff --git a/src/iceberg/table_scan.h b/src/iceberg/table_scan.h index 6c99e4c5f..381456a51 100644 --- a/src/iceberg/table_scan.h +++ b/src/iceberg/table_scan.h @@ -468,8 +468,7 @@ class ICEBERG_EXPORT DataTableScan : public TableScan { /// /// Unlike PlanFiles(), this method does not materialize all manifest entries and scan /// tasks. The iterator owns its planning resources and can outlive this scan. - Result>>> PlanFilesIterator() - const; + Result PlanFilesIterator() const; private: Status ReportScan(const Snapshot& snapshot, const ScanMetrics& scan_metrics) const; diff --git a/src/iceberg/type_fwd.h b/src/iceberg/type_fwd.h index 83fcb8051..d6e978200 100644 --- a/src/iceberg/type_fwd.h +++ b/src/iceberg/type_fwd.h @@ -24,6 +24,8 @@ /// you can include this instead of the "full" headers to help reduce compile /// times. +#include + namespace iceberg { /// \brief A data type. @@ -232,6 +234,8 @@ struct SessionContext; class Executor; template class Iterator; +using FileScanTaskIterator = + std::unique_ptr>>; /// \brief Metrics reporting. class MetricsReporter; From 93388cc9f1d306e7f26983234ca3b21f0d0ba460 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Mon, 7 Sep 2026 11:15:22 +0800 Subject: [PATCH 12/21] style: fix formatting --- src/iceberg/manifest/manifest_group.cc | 3 +-- src/iceberg/table_scan.cc | 9 ++++----- src/iceberg/type_fwd.h | 3 +-- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/iceberg/manifest/manifest_group.cc b/src/iceberg/manifest/manifest_group.cc index ef0475573..e7431ab44 100644 --- a/src/iceberg/manifest/manifest_group.cc +++ b/src/iceberg/manifest/manifest_group.cc @@ -134,8 +134,7 @@ ManifestGroup& ManifestGroup::operator=(ManifestGroup&&) noexcept = default; class ManifestGroup::FilePlanningIterator final : public Iterator> { public: - static Result Make( - std::unique_ptr group) { + static Result Make(std::unique_ptr group) { ICEBERG_RETURN_UNEXPECTED(group->CheckErrors()); group->delete_index_builder_.WithScanMetrics(group->scan_metrics_); diff --git a/src/iceberg/table_scan.cc b/src/iceberg/table_scan.cc index 5448f3050..8af733a7c 100644 --- a/src/iceberg/table_scan.cc +++ b/src/iceberg/table_scan.cc @@ -109,11 +109,10 @@ Result MakeScanReport(const DataTableScan& scan, const Snapshot& sna class ReportingFileTaskIterator final : public Iterator> { public: - ReportingFileTaskIterator( - FileScanTaskIterator iterator, - std::shared_ptr scan_metrics, - std::chrono::nanoseconds planning_duration, - std::shared_ptr reporter, ScanReport report) + ReportingFileTaskIterator(FileScanTaskIterator iterator, + std::shared_ptr scan_metrics, + std::chrono::nanoseconds planning_duration, + std::shared_ptr reporter, ScanReport report) : iterator_(std::move(iterator)), scan_metrics_(std::move(scan_metrics)), planning_duration_(std::move(planning_duration)), diff --git a/src/iceberg/type_fwd.h b/src/iceberg/type_fwd.h index d6e978200..ff0636c17 100644 --- a/src/iceberg/type_fwd.h +++ b/src/iceberg/type_fwd.h @@ -234,8 +234,7 @@ struct SessionContext; class Executor; template class Iterator; -using FileScanTaskIterator = - std::unique_ptr>>; +using FileScanTaskIterator = std::unique_ptr>>; /// \brief Metrics reporting. class MetricsReporter; From d82d9e2e3fa94bc45ce83e88a99bad0416931574 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Mon, 7 Sep 2026 11:27:59 +0800 Subject: [PATCH 13/21] fix: make file scan iterator type self-contained --- src/iceberg/file_scan_task_iterator.h | 35 +++++++++++++++++++++++++++ src/iceberg/manifest/manifest_group.h | 2 +- src/iceberg/meson.build | 1 + src/iceberg/table_scan.h | 2 +- src/iceberg/test/iterator_test.cc | 8 ++++-- src/iceberg/type_fwd.h | 3 --- 6 files changed, 44 insertions(+), 7 deletions(-) create mode 100644 src/iceberg/file_scan_task_iterator.h diff --git a/src/iceberg/file_scan_task_iterator.h b/src/iceberg/file_scan_task_iterator.h new file mode 100644 index 000000000..612d788f8 --- /dev/null +++ b/src/iceberg/file_scan_task_iterator.h @@ -0,0 +1,35 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +/// \file iceberg/file_scan_task_iterator.h +/// \brief Define the owning iterator type for file scan tasks. + +#include + +#include "iceberg/util/iterator.h" + +namespace iceberg { + +class FileScanTask; + +using FileScanTaskIterator = std::unique_ptr>>; + +} // namespace iceberg diff --git a/src/iceberg/manifest/manifest_group.h b/src/iceberg/manifest/manifest_group.h index 107e684e1..cd613e998 100644 --- a/src/iceberg/manifest/manifest_group.h +++ b/src/iceberg/manifest/manifest_group.h @@ -30,6 +30,7 @@ #include #include "iceberg/delete_file_index.h" +#include "iceberg/file_scan_task_iterator.h" #include "iceberg/iceberg_export.h" #include "iceberg/manifest/manifest_entry.h" #include "iceberg/manifest/manifest_list.h" @@ -37,7 +38,6 @@ #include "iceberg/type_fwd.h" #include "iceberg/util/error_collector.h" #include "iceberg/util/executor.h" -#include "iceberg/util/iterator.h" namespace iceberg { diff --git a/src/iceberg/meson.build b/src/iceberg/meson.build index 989f4ae03..62bd98448 100644 --- a/src/iceberg/meson.build +++ b/src/iceberg/meson.build @@ -326,6 +326,7 @@ install_headers( 'file_io.h', 'file_io_registry.h', 'file_reader.h', + 'file_scan_task_iterator.h', 'file_writer.h', 'geospatial.h', 'iceberg_data_export.h', diff --git a/src/iceberg/table_scan.h b/src/iceberg/table_scan.h index 381456a51..af365b4c6 100644 --- a/src/iceberg/table_scan.h +++ b/src/iceberg/table_scan.h @@ -30,13 +30,13 @@ #include #include +#include "iceberg/file_scan_task_iterator.h" #include "iceberg/iceberg_export.h" #include "iceberg/result.h" #include "iceberg/table_metadata.h" #include "iceberg/type_fwd.h" #include "iceberg/util/error_collector.h" #include "iceberg/util/executor.h" -#include "iceberg/util/iterator.h" namespace iceberg { diff --git a/src/iceberg/test/iterator_test.cc b/src/iceberg/test/iterator_test.cc index 49b338697..ad8c83dd5 100644 --- a/src/iceberg/test/iterator_test.cc +++ b/src/iceberg/test/iterator_test.cc @@ -17,8 +17,6 @@ * under the License. */ -#include "iceberg/util/iterator.h" - #include #include #include @@ -26,6 +24,7 @@ #include +#include "iceberg/file_scan_task_iterator.h" #include "iceberg/test/matchers.h" namespace iceberg { @@ -104,6 +103,11 @@ static_assert(std::is_move_assignable_v); static_assert(std::is_move_constructible_v); static_assert(std::is_move_assignable_v); +TEST(IteratorTest, FileScanTaskIteratorSupportsIncompleteFileScanTask) { + FileScanTaskIterator iterator; + EXPECT_EQ(iterator, nullptr); +} + TEST(IteratorTest, ToVectorSupportsCopyOnlyValues) { CopyOnlyIterator iterator; diff --git a/src/iceberg/type_fwd.h b/src/iceberg/type_fwd.h index ff0636c17..83fcb8051 100644 --- a/src/iceberg/type_fwd.h +++ b/src/iceberg/type_fwd.h @@ -24,8 +24,6 @@ /// you can include this instead of the "full" headers to help reduce compile /// times. -#include - namespace iceberg { /// \brief A data type. @@ -234,7 +232,6 @@ struct SessionContext; class Executor; template class Iterator; -using FileScanTaskIterator = std::unique_ptr>>; /// \brief Metrics reporting. class MetricsReporter; From 0113cfbf5387c4e1b2a6cda901d1efcbcb072662 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Thu, 10 Sep 2026 21:52:51 +0800 Subject: [PATCH 14/21] fix: address stream API review feedback Co-authored-by: Codex --- example/demo_example.cc | 2 +- ...ask_iterator.h => file_scan_task_stream.h} | 8 +-- src/iceberg/manifest/manifest_group.cc | 69 +++++++++++-------- src/iceberg/manifest/manifest_group.h | 27 +++++--- src/iceberg/manifest/manifest_reader.cc | 27 ++++---- src/iceberg/manifest/manifest_reader.h | 30 ++++---- .../manifest/manifest_reader_internal.h | 8 +-- src/iceberg/meson.build | 2 +- src/iceberg/table_scan.cc | 21 ++++-- src/iceberg/table_scan.h | 7 +- src/iceberg/test/iterator_test.cc | 6 +- src/iceberg/test/manifest_group_test.cc | 50 ++++++++++++-- src/iceberg/test/manifest_reader_test.cc | 10 +-- .../test/scan_planning_metrics_test.cc | 4 +- src/iceberg/test/table_scan_test.cc | 6 +- src/iceberg/type_fwd.h | 2 +- 16 files changed, 174 insertions(+), 105 deletions(-) rename src/iceberg/{file_scan_task_iterator.h => file_scan_task_stream.h} (78%) diff --git a/example/demo_example.cc b/example/demo_example.cc index a477e5af1..6098a98ff 100644 --- a/example/demo_example.cc +++ b/example/demo_example.cc @@ -79,7 +79,7 @@ int main(int argc, char** argv) { } auto scan = std::move(scan_result.value()); - auto plan_result = scan->PlanFilesIterator(); + auto plan_result = scan->PlanFilesStream(); if (!plan_result.has_value()) { std::cerr << "Failed to plan files: " << plan_result.error().message << std::endl; return 1; diff --git a/src/iceberg/file_scan_task_iterator.h b/src/iceberg/file_scan_task_stream.h similarity index 78% rename from src/iceberg/file_scan_task_iterator.h rename to src/iceberg/file_scan_task_stream.h index 612d788f8..307c907b1 100644 --- a/src/iceberg/file_scan_task_iterator.h +++ b/src/iceberg/file_scan_task_stream.h @@ -3,7 +3,7 @@ * or more contributor license agreements. See the NOTICE file * distributed with this work for additional information * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the + * under the Apache License, Version 2.0 (the * "License"); you may not use this file except in compliance * with the License. You may obtain a copy of the License at * @@ -19,8 +19,8 @@ #pragma once -/// \file iceberg/file_scan_task_iterator.h -/// \brief Define the owning iterator type for file scan tasks. +/// \file iceberg/file_scan_task_stream.h +/// \brief Define the owning stream type for file scan tasks. #include @@ -30,6 +30,6 @@ namespace iceberg { class FileScanTask; -using FileScanTaskIterator = std::unique_ptr>>; +using FileScanTaskStream = std::unique_ptr>>; } // namespace iceberg diff --git a/src/iceberg/manifest/manifest_group.cc b/src/iceberg/manifest/manifest_group.cc index e7431ab44..7af5315f1 100644 --- a/src/iceberg/manifest/manifest_group.cc +++ b/src/iceberg/manifest/manifest_group.cc @@ -134,13 +134,13 @@ ManifestGroup& ManifestGroup::operator=(ManifestGroup&&) noexcept = default; class ManifestGroup::FilePlanningIterator final : public Iterator> { public: - static Result Make(std::unique_ptr group) { + static Result Make(std::unique_ptr group) { ICEBERG_RETURN_UNEXPECTED(group->CheckErrors()); group->delete_index_builder_.WithScanMetrics(group->scan_metrics_); ICEBERG_ASSIGN_OR_RAISE(auto delete_index, group->delete_index_builder_.Build()); - const bool drop_stats = + auto stats_projection = group->PrepareStatsProjection(delete_index->has_equality_deletes()); std::unique_ptr data_file_evaluator; @@ -151,10 +151,11 @@ class ManifestGroup::FilePlanningIterator final Evaluator::Make(*DataFileFilterSchema(), group->file_filter_, group->case_sensitive_)); } + const bool drop_stats = stats_projection.drop_stats; - return FileScanTaskIterator( - new FilePlanningIterator(std::move(group), std::move(delete_index), - std::move(data_file_evaluator), drop_stats)); + return FileScanTaskStream(new FilePlanningIterator( + std::move(group), std::move(delete_index), std::move(data_file_evaluator), + std::move(stats_projection.columns), drop_stats)); } Result>> NextImpl() override { @@ -211,10 +212,12 @@ class ManifestGroup::FilePlanningIterator final private: FilePlanningIterator(std::unique_ptr group, std::unique_ptr delete_index, - std::unique_ptr data_file_evaluator, bool drop_stats) + std::unique_ptr data_file_evaluator, + std::vector columns, bool drop_stats) : group_(std::move(group)), delete_index_(std::move(delete_index)), data_file_evaluator_(std::move(data_file_evaluator)), + columns_(std::move(columns)), drop_stats_(drop_stats) {} using TaggedEntry = std::pair; @@ -335,10 +338,10 @@ class ManifestGroup::FilePlanningIterator final continue; } - ICEBERG_ASSIGN_OR_RAISE(auto reader, group_->MakeReader(manifest)); + ICEBERG_ASSIGN_OR_RAISE(auto reader, group_->MakeReader(manifest, columns_)); ICEBERG_ASSIGN_OR_RAISE(entry_iterator_, group_->ignore_deleted_ - ? reader->LiveEntriesIterator() - : reader->EntriesIterator()); + ? reader->LiveEntriesStream() + : reader->EntriesStream()); current_spec_id_ = manifest.partition_spec_id; return true; } @@ -369,10 +372,11 @@ class ManifestGroup::FilePlanningIterator final ParallelCollect( group_->executor_, manifests, [this](const ManifestFile* manifest) -> Result> { - ICEBERG_ASSIGN_OR_RAISE(auto reader, group_->MakeReader(*manifest)); + ICEBERG_ASSIGN_OR_RAISE(auto reader, + group_->MakeReader(*manifest, columns_)); ICEBERG_ASSIGN_OR_RAISE(auto iterator, group_->ignore_deleted_ - ? reader->LiveEntriesIterator() - : reader->EntriesIterator()); + ? reader->LiveEntriesStream() + : reader->EntriesStream()); std::vector tagged_iterators; tagged_iterators.emplace_back(manifest->partition_spec_id, @@ -416,6 +420,7 @@ class ManifestGroup::FilePlanningIterator final std::unique_ptr group_; std::unique_ptr delete_index_; std::unique_ptr data_file_evaluator_; + std::vector columns_; std::unordered_map> manifest_evaluators_; std::unordered_map> residual_evaluators_; std::unique_ptr> entry_iterator_; @@ -555,7 +560,7 @@ Result>> ManifestGroup::PlanFiles() { return file_tasks; } -Result ManifestGroup::PlanFilesIterator() && { +Result ManifestGroup::PlanFilesStream() && { auto group = std::make_unique(std::move(*this)); return FilePlanningIterator::Make(std::move(group)); } @@ -585,7 +590,7 @@ Result>> ManifestGroup::Plan( delete_index_builder_.WithScanMetrics(scan_metrics_); ICEBERG_ASSIGN_OR_RAISE(auto delete_index, delete_index_builder_.Build()); - const bool drop_stats = PrepareStatsProjection(delete_index->has_equality_deletes()); + auto stats_projection = PrepareStatsProjection(delete_index->has_equality_deletes()); std::unordered_map> task_context_cache; auto get_task_context = [&](int32_t spec_id) -> Result { @@ -603,13 +608,13 @@ Result>> ManifestGroup::Plan( TaskContext{.spec = spec, .deletes = delete_index.get(), .residuals = residuals, - .drop_stats = drop_stats, + .drop_stats = stats_projection.drop_stats, .columns_to_keep_stats = columns_to_keep_stats_}); return task_context_cache[spec_id].get(); }; - ICEBERG_ASSIGN_OR_RAISE(auto entry_groups, ReadEntries()); + ICEBERG_ASSIGN_OR_RAISE(auto entry_groups, ReadEntries(stats_projection.columns)); std::vector> all_tasks; for (auto& [spec_id, entries] : entry_groups) { @@ -623,7 +628,7 @@ Result>> ManifestGroup::Plan( } Result> ManifestGroup::Entries() { - ICEBERG_ASSIGN_OR_RAISE(auto entry_groups, ReadEntries()); + ICEBERG_ASSIGN_OR_RAISE(auto entry_groups, ReadEntries(columns_)); std::vector all_entries; for (auto& [_, entries] : entry_groups) { @@ -635,13 +640,14 @@ Result> ManifestGroup::Entries() { } Result> ManifestGroup::MakeReader( - const ManifestFile& manifest) { + const ManifestFile& manifest, const std::vector& columns) { ICEBERG_ASSIGN_OR_RAISE(auto reader, ManifestReader::Make(manifest, io_, schema_, specs_by_id_)); - auto columns = columns_; + auto reader_columns = columns; if (file_filter_ && file_filter_->op() != Expression::Operation::kTrue && - !columns.empty() && !std::ranges::contains(columns, Schema::kAllColumns)) { + !reader_columns.empty() && + !std::ranges::contains(reader_columns, Schema::kAllColumns)) { auto data_file_schema = DataFileFilterSchema(); ICEBERG_ASSIGN_OR_RAISE( auto bound_file_filter, @@ -649,7 +655,8 @@ Result> ManifestGroup::MakeReader( ICEBERG_ASSIGN_OR_RAISE(auto referenced_field_ids, ReferenceVisitor::GetReferencedFieldIds(bound_file_filter)); - std::unordered_set selected_columns(columns.cbegin(), columns.cend()); + std::unordered_set selected_columns(reader_columns.cbegin(), + reader_columns.cend()); for (const auto field_id : referenced_field_ids) { if (field_id == DataFile::kSpecIdFieldId) { continue; @@ -661,8 +668,8 @@ Result> ManifestGroup::MakeReader( if (selected_columns.contains(column_name_str)) { continue; } - columns.push_back(std::move(column_name_str)); - selected_columns.insert(columns.back()); + reader_columns.push_back(std::move(column_name_str)); + selected_columns.insert(reader_columns.back()); } } } @@ -670,7 +677,7 @@ Result> ManifestGroup::MakeReader( reader->FilterRows(data_filter_) .FilterPartitions(partition_filter_) .CaseSensitive(case_sensitive_) - .Select(std::move(columns)); + .Select(std::move(reader_columns)); if (scan_metrics_) { reader->SkipCounter(scan_metrics_->skipped_data_files); @@ -679,20 +686,22 @@ Result> ManifestGroup::MakeReader( return reader; } -bool ManifestGroup::PrepareStatsProjection(bool has_equality_deletes) { +ManifestGroup::StatsProjection ManifestGroup::PrepareStatsProjection( + bool has_equality_deletes) const { // The caller's projection records whether stats were requested. Equality-delete // matching may add stats temporarily, but they should still be dropped from the // result when the original projection did not request them. Keeping this decision // here ensures eager and iterator planning use identical semantics. - const bool drop_stats = ManifestReader::ShouldDropStats(columns_); + StatsProjection result{.columns = columns_, + .drop_stats = ManifestReader::ShouldDropStats(columns_)}; if (has_equality_deletes) { - columns_ = ManifestReader::WithStatsColumns(columns_); + result.columns = ManifestReader::WithStatsColumns(result.columns); } - return drop_stats; + return result; } Result>> -ManifestGroup::ReadEntries() { +ManifestGroup::ReadEntries(const std::vector& columns) { const auto cache_capacity = static_cast(specs_by_id_.size()); auto get_manifest_evaluator = internal::MemoizeLru( [this](int32_t spec_id) -> Result> { @@ -759,7 +768,7 @@ ManifestGroup::ReadEntries() { } // Read manifest entries - ICEBERG_ASSIGN_OR_RAISE(auto reader, MakeReader(manifest)); + ICEBERG_ASSIGN_OR_RAISE(auto reader, MakeReader(manifest, columns)); ICEBERG_ASSIGN_OR_RAISE( auto entries, ignore_deleted_ ? reader->LiveEntries() : reader->Entries()); diff --git a/src/iceberg/manifest/manifest_group.h b/src/iceberg/manifest/manifest_group.h index cd613e998..f0cb5e360 100644 --- a/src/iceberg/manifest/manifest_group.h +++ b/src/iceberg/manifest/manifest_group.h @@ -30,7 +30,7 @@ #include #include "iceberg/delete_file_index.h" -#include "iceberg/file_scan_task_iterator.h" +#include "iceberg/file_scan_task_stream.h" #include "iceberg/iceberg_export.h" #include "iceberg/manifest/manifest_entry.h" #include "iceberg/manifest/manifest_list.h" @@ -139,13 +139,15 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector { /// \brief Lazily plan scan tasks for matching data files. /// - /// The returned iterator owns the planning state and may outlive this ManifestGroup. + /// The returned stream owns the planning state and may outlive this ManifestGroup. /// It reads one bounded manifest batch at a time instead of materializing all manifest - /// entries and scan tasks. When PlanWith() configures an executor, entry iterators for + /// entries and scan tasks. When PlanWith() configures an executor, entry streams for /// manifests in each batch are opened in parallel, while entries are consumed one - /// manifest at a time. Creating the iterator consumes this group's configuration, so - /// this method may only be called on an rvalue. - Result PlanFilesIterator() &&; + /// manifest at a time. Delete manifests are still read eagerly when creating the + /// stream because delete files must be indexed before data-file planning can begin. + /// Creating the stream consumes this group's configuration, so this method may only + /// be called on an rvalue. + Result PlanFilesStream() &&; /// \brief Get all matching manifest entries. Result> Entries(); @@ -164,16 +166,23 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector { private: class FilePlanningIterator; + struct StatsProjection { + std::vector columns; + bool drop_stats; + }; + ManifestGroup(std::shared_ptr io, std::shared_ptr schema, std::unordered_map> specs_by_id, std::vector data_manifests, DeleteFileIndex::Builder&& delete_index_builder); - Result>> ReadEntries(); + Result>> ReadEntries( + const std::vector& columns); - Result> MakeReader(const ManifestFile& manifest); + Result> MakeReader( + const ManifestFile& manifest, const std::vector& columns); - bool PrepareStatsProjection(bool has_equality_deletes); + StatsProjection PrepareStatsProjection(bool has_equality_deletes) const; std::shared_ptr io_; std::shared_ptr schema_; diff --git a/src/iceberg/manifest/manifest_reader.cc b/src/iceberg/manifest/manifest_reader.cc index d9e5a0de7..f746d2904 100644 --- a/src/iceberg/manifest/manifest_reader.cc +++ b/src/iceberg/manifest/manifest_reader.cc @@ -815,17 +815,17 @@ class ManifestEntryIteratorImpl final : public Iterator { } // namespace -Result>> ManifestReader::EntriesIterator() { - if (auto* iterable = dynamic_cast(this)) { - return iterable->EntriesIterator(); +Result>> ManifestReader::EntriesStream() { + if (auto* iterable = dynamic_cast(this)) { + return iterable->EntriesStream(); } ICEBERG_ASSIGN_OR_RAISE(auto entries, Entries()); return std::make_unique>(std::move(entries)); } -Result>> ManifestReader::LiveEntriesIterator() { - if (auto* iterable = dynamic_cast(this)) { - return iterable->LiveEntriesIterator(); +Result>> ManifestReader::LiveEntriesStream() { + if (auto* iterable = dynamic_cast(this)) { + return iterable->LiveEntriesStream(); } ICEBERG_ASSIGN_OR_RAISE(auto entries, LiveEntries()); return std::make_unique>(std::move(entries)); @@ -1002,25 +1002,24 @@ Status ManifestReaderImpl::OpenReader(std::shared_ptr projection) { } Result> ManifestReaderImpl::Entries() { - ICEBERG_ASSIGN_OR_RAISE(auto entries, EntriesIterator()); + ICEBERG_ASSIGN_OR_RAISE(auto entries, EntriesStream()); return entries->ToVector(); } Result> ManifestReaderImpl::LiveEntries() { - ICEBERG_ASSIGN_OR_RAISE(auto entries, LiveEntriesIterator()); + ICEBERG_ASSIGN_OR_RAISE(auto entries, LiveEntriesStream()); return entries->ToVector(); } -Result>> ManifestReaderImpl::EntriesIterator() { - return MakeEntriesIterator(/*only_live=*/false); +Result>> ManifestReaderImpl::EntriesStream() { + return MakeEntriesStream(/*only_live=*/false); } -Result>> -ManifestReaderImpl::LiveEntriesIterator() { - return MakeEntriesIterator(/*only_live=*/true); +Result>> ManifestReaderImpl::LiveEntriesStream() { + return MakeEntriesStream(/*only_live=*/true); } -Result>> ManifestReaderImpl::MakeEntriesIterator( +Result>> ManifestReaderImpl::MakeEntriesStream( bool only_live) { ICEBERG_ASSIGN_OR_RAISE(auto partition_type, spec_->RawPartitionType(*schema_)); auto data_file_schema = DataFile::Type(std::move(partition_type))->ToSchema(); diff --git a/src/iceberg/manifest/manifest_reader.h b/src/iceberg/manifest/manifest_reader.h index 163c482b0..5d16795f7 100644 --- a/src/iceberg/manifest/manifest_reader.h +++ b/src/iceberg/manifest/manifest_reader.h @@ -50,15 +50,17 @@ class ICEBERG_EXPORT ManifestReader { /// \brief Lazily read manifest entries. /// - /// Implementations using SupportsManifestEntryIteration stream entries lazily. Other - /// implementations are adapted from Entries() for compatibility. - Result>> EntriesIterator(); + /// Implementations using SupportsManifestEntryStreaming produce entries lazily. Other + /// implementations are adapted from Entries() for compatibility. The returned stream + /// is fallible and single-pass. + Result>> EntriesStream(); /// \brief Lazily read only live (non-deleted) manifest entries. /// - /// Implementations using SupportsManifestEntryIteration stream entries lazily. Other - /// implementations are adapted from LiveEntries() for compatibility. - Result>> LiveEntriesIterator(); + /// Implementations using SupportsManifestEntryStreaming produce entries lazily. Other + /// implementations are adapted from LiveEntries() for compatibility. The returned + /// stream is fallible and single-pass. + Result>> LiveEntriesStream(); /// \brief Select specific columns of data file to read from the manifest entries. /// @@ -145,23 +147,23 @@ class ICEBERG_EXPORT ManifestReader { const std::vector& columns); }; -/// \brief Optional mix-in for ManifestReader implementations that support lazy entry -/// iteration. -class ICEBERG_EXPORT SupportsManifestEntryIteration { +/// \brief Optional mix-in for ManifestReader implementations that support entry +/// streaming. +class ICEBERG_EXPORT SupportsManifestEntryStreaming { public: - virtual ~SupportsManifestEntryIteration() = default; + virtual ~SupportsManifestEntryStreaming() = default; /// \brief Lazily read manifest entries. /// - /// The returned iterator must own all resources required for iteration and must not + /// The returned stream must own all resources required for iteration and must not /// depend on this reader remaining alive. - virtual Result>> EntriesIterator() = 0; + virtual Result>> EntriesStream() = 0; /// \brief Lazily read only live (non-deleted) manifest entries. /// - /// The returned iterator must own all resources required for iteration and must not + /// The returned stream must own all resources required for iteration and must not /// depend on this reader remaining alive. - virtual Result>> LiveEntriesIterator() = 0; + virtual Result>> LiveEntriesStream() = 0; }; /// \brief Read manifest files from a manifest list file. diff --git a/src/iceberg/manifest/manifest_reader_internal.h b/src/iceberg/manifest/manifest_reader_internal.h index 9b9352a24..f6c4b1301 100644 --- a/src/iceberg/manifest/manifest_reader_internal.h +++ b/src/iceberg/manifest/manifest_reader_internal.h @@ -43,7 +43,7 @@ namespace iceberg { /// This implementation supports lazy reader creation and filtering based on /// partition expressions, row expressions, and partition sets. Following the /// Java implementation pattern. -class ManifestReaderImpl : public ManifestReader, public SupportsManifestEntryIteration { +class ManifestReaderImpl : public ManifestReader, public SupportsManifestEntryStreaming { public: /// \brief Construct a ManifestReaderImpl for lazy initialization. /// @@ -66,9 +66,9 @@ class ManifestReaderImpl : public ManifestReader, public SupportsManifestEntryIt Result> LiveEntries() override; - Result>> EntriesIterator() override; + Result>> EntriesStream() override; - Result>> LiveEntriesIterator() override; + Result>> LiveEntriesStream() override; ManifestReader& Select(const std::vector& columns) override; @@ -86,7 +86,7 @@ class ManifestReaderImpl : public ManifestReader, public SupportsManifestEntryIt private: /// \brief Create an entry iterator with optional live-only filtering. - Result>> MakeEntriesIterator(bool only_live); + Result>> MakeEntriesStream(bool only_live); /// \brief Lazily open the underlying Avro reader with appropriate schema projection. Status OpenReader(std::shared_ptr projection); diff --git a/src/iceberg/meson.build b/src/iceberg/meson.build index 62bd98448..2f236a106 100644 --- a/src/iceberg/meson.build +++ b/src/iceberg/meson.build @@ -326,7 +326,7 @@ install_headers( 'file_io.h', 'file_io_registry.h', 'file_reader.h', - 'file_scan_task_iterator.h', + 'file_scan_task_stream.h', 'file_writer.h', 'geospatial.h', 'iceberg_data_export.h', diff --git a/src/iceberg/table_scan.cc b/src/iceberg/table_scan.cc index 8af733a7c..2bcaad44b 100644 --- a/src/iceberg/table_scan.cc +++ b/src/iceberg/table_scan.cc @@ -22,6 +22,7 @@ #include #include #include +#include #include #include "iceberg/expression/binder.h" @@ -109,7 +110,7 @@ Result MakeScanReport(const DataTableScan& scan, const Snapshot& sna class ReportingFileTaskIterator final : public Iterator> { public: - ReportingFileTaskIterator(FileScanTaskIterator iterator, + ReportingFileTaskIterator(FileScanTaskStream iterator, std::shared_ptr scan_metrics, std::chrono::nanoseconds planning_duration, std::shared_ptr reporter, ScanReport report) @@ -146,7 +147,7 @@ class ReportingFileTaskIterator final : public IteratorReport(report_); } - FileScanTaskIterator iterator_; + FileScanTaskStream iterator_; std::shared_ptr scan_metrics_; std::chrono::nanoseconds planning_duration_; std::shared_ptr reporter_; @@ -720,7 +721,7 @@ Result>> DataTableScan::PlanFiles() co return tasks; } -Result DataTableScan::PlanFilesIterator() const { +Result DataTableScan::PlanFilesStream() const { ICEBERG_ASSIGN_OR_RAISE(auto snapshot, this->snapshot()); if (!snapshot) { return std::make_unique>>(); @@ -748,11 +749,17 @@ Result DataTableScan::PlanFilesIterator() const { static_cast(delete_manifests.size())); } + std::vector owned_data_manifests( + std::make_move_iterator(data_manifests.begin()), + std::make_move_iterator(data_manifests.end())); + std::vector owned_delete_manifests( + std::make_move_iterator(delete_manifests.begin()), + std::make_move_iterator(delete_manifests.end())); + ICEBERG_ASSIGN_OR_RAISE( auto manifest_group, - ManifestGroup::Make(io_, schema_, specs_by_id, - {data_manifests.begin(), data_manifests.end()}, - {delete_manifests.begin(), delete_manifests.end()})); + ManifestGroup::Make(io_, schema_, specs_by_id, std::move(owned_data_manifests), + std::move(owned_delete_manifests))); manifest_group->CaseSensitive(context_.case_sensitive) .Select(ScanColumns()) .FilterData(filter()) @@ -764,7 +771,7 @@ Result DataTableScan::PlanFilesIterator() const { manifest_group->IgnoreResiduals(); } - ICEBERG_ASSIGN_OR_RAISE(auto iterator, std::move(*manifest_group).PlanFilesIterator()); + ICEBERG_ASSIGN_OR_RAISE(auto iterator, std::move(*manifest_group).PlanFilesStream()); if (!planning_start.has_value()) { return iterator; } diff --git a/src/iceberg/table_scan.h b/src/iceberg/table_scan.h index af365b4c6..b68bae05f 100644 --- a/src/iceberg/table_scan.h +++ b/src/iceberg/table_scan.h @@ -30,7 +30,7 @@ #include #include -#include "iceberg/file_scan_task_iterator.h" +#include "iceberg/file_scan_task_stream.h" #include "iceberg/iceberg_export.h" #include "iceberg/result.h" #include "iceberg/table_metadata.h" @@ -467,8 +467,9 @@ class ICEBERG_EXPORT DataTableScan : public TableScan { /// \brief Lazily plans scan tasks by resolving manifests and data files on demand. /// /// Unlike PlanFiles(), this method does not materialize all manifest entries and scan - /// tasks. The iterator owns its planning resources and can outlive this scan. - Result PlanFilesIterator() const; + /// tasks. The returned fallible, single-pass stream owns its planning resources and + /// can outlive this scan. + Result PlanFilesStream() const; private: Status ReportScan(const Snapshot& snapshot, const ScanMetrics& scan_metrics) const; diff --git a/src/iceberg/test/iterator_test.cc b/src/iceberg/test/iterator_test.cc index ad8c83dd5..ffc21bc60 100644 --- a/src/iceberg/test/iterator_test.cc +++ b/src/iceberg/test/iterator_test.cc @@ -24,7 +24,7 @@ #include -#include "iceberg/file_scan_task_iterator.h" +#include "iceberg/file_scan_task_stream.h" #include "iceberg/test/matchers.h" namespace iceberg { @@ -103,8 +103,8 @@ static_assert(std::is_move_assignable_v); static_assert(std::is_move_constructible_v); static_assert(std::is_move_assignable_v); -TEST(IteratorTest, FileScanTaskIteratorSupportsIncompleteFileScanTask) { - FileScanTaskIterator iterator; +TEST(IteratorTest, FileScanTaskStreamSupportsIncompleteFileScanTask) { + FileScanTaskStream iterator; EXPECT_EQ(iterator, nullptr); } diff --git a/src/iceberg/test/manifest_group_test.cc b/src/iceberg/test/manifest_group_test.cc index 797ca7b19..d6b742f44 100644 --- a/src/iceberg/test/manifest_group_test.cc +++ b/src/iceberg/test/manifest_group_test.cc @@ -292,7 +292,7 @@ TEST_P(ManifestGroupTest, CreateAndGetEntries) { EXPECT_EQ(tasks[1]->delete_files()[0]->file_path, "/path/to/delete.parquet"); } -TEST_P(ManifestGroupTest, PlanFilesIteratorPreservesSelectAllWithEqualityDeletes) { +TEST_P(ManifestGroupTest, PlanFilesStreamPreservesSelectAllWithEqualityDeletes) { auto version = GetParam(); if (version < 2) { GTEST_SKIP() << "Delete files only supported in V2+"; @@ -323,7 +323,7 @@ TEST_P(ManifestGroupTest, PlanFilesIteratorPreservesSelectAllWithEqualityDeletes ICEBERG_UNWRAP_OR_FAIL( auto group, ManifestGroup::Make(file_io_, schema_, GetSpecsById(), {data_manifest}, {delete_manifest})); - ICEBERG_UNWRAP_OR_FAIL(auto iterator, std::move(*group).PlanFilesIterator()); + ICEBERG_UNWRAP_OR_FAIL(auto iterator, std::move(*group).PlanFilesStream()); ICEBERG_UNWRAP_OR_FAIL(auto task, iterator->Next()); ASSERT_TRUE(task.has_value()); @@ -339,6 +339,48 @@ TEST_P(ManifestGroupTest, PlanFilesIteratorPreservesSelectAllWithEqualityDeletes EXPECT_FALSE(end.has_value()); } +TEST_P(ManifestGroupTest, PlanFilesKeepsStatsProjectionLocal) { + auto version = GetParam(); + if (version < 2) { + GTEST_SKIP() << "Equality deletes only supported in V2+"; + } + + constexpr int64_t kSnapshotId = 1000L; + const auto part_value = PartitionValues({Literal::Int(0)}); + + auto data_file = MakeDataFile("/path/to/data.parquet", part_value, + partitioned_spec_->spec_id(), /*record_count=*/100); + data_file->lower_bounds[1] = Literal::Int(20).Serialize().value(); + data_file->upper_bounds[1] = Literal::Int(30).Serialize().value(); + auto data_manifest = + WriteDataManifest(version, kSnapshotId, + {MakeEntry(ManifestStatus::kAdded, kSnapshotId, + /*sequence_number=*/1, std::move(data_file))}, + partitioned_spec_); + + auto equality_delete = MakeEqualityDeleteFile("/path/to/equality-delete.parquet", + part_value, partitioned_spec_->spec_id()); + equality_delete->lower_bounds[1] = Literal::Int(20).Serialize().value(); + equality_delete->upper_bounds[1] = Literal::Int(30).Serialize().value(); + auto delete_manifest = + WriteDeleteManifest(version, kSnapshotId, + {MakeEntry(ManifestStatus::kAdded, kSnapshotId, + /*sequence_number=*/2, std::move(equality_delete))}, + partitioned_spec_); + + ICEBERG_UNWRAP_OR_FAIL( + auto group, ManifestGroup::Make(file_io_, schema_, GetSpecsById(), {data_manifest}, + {delete_manifest})); + group->Select({"file_path"}); + + for (int attempt = 0; attempt < 2; ++attempt) { + ICEBERG_UNWRAP_OR_FAIL(auto tasks, group->PlanFiles()); + ASSERT_EQ(tasks.size(), 1); + EXPECT_TRUE(tasks.front()->data_file()->lower_bounds.empty()); + EXPECT_TRUE(tasks.front()->data_file()->upper_bounds.empty()); + } +} + TEST_P(ManifestGroupTest, IgnoreDeleted) { auto version = GetParam(); @@ -684,7 +726,7 @@ TEST_P(ManifestGroupTest, MultipleDataManifests) { EXPECT_EQ(executor.submit_count(), 2); } -TEST_P(ManifestGroupTest, PlanFilesIteratorUsesExecutor) { +TEST_P(ManifestGroupTest, PlanFilesStreamUsesExecutor) { auto version = GetParam(); const auto partition_a = PartitionValues({Literal::Int(0)}); @@ -711,7 +753,7 @@ TEST_P(ManifestGroupTest, PlanFilesIteratorUsesExecutor) { test::ThreadExecutor executor; group->PlanWith(std::ref(executor)); - ICEBERG_UNWRAP_OR_FAIL(auto iterator, std::move(*group).PlanFilesIterator()); + ICEBERG_UNWRAP_OR_FAIL(auto iterator, std::move(*group).PlanFilesStream()); ICEBERG_UNWRAP_OR_FAIL(auto tasks, iterator->ToVector()); EXPECT_THAT(GetPaths(tasks), testing::UnorderedElementsAre("/path/to/data1.parquet", diff --git a/src/iceberg/test/manifest_reader_test.cc b/src/iceberg/test/manifest_reader_test.cc index af96ca971..a2c30be4e 100644 --- a/src/iceberg/test/manifest_reader_test.cc +++ b/src/iceberg/test/manifest_reader_test.cc @@ -85,16 +85,16 @@ class LegacyManifestReader : public ManifestReader { int live_entries_calls = 0; }; -TEST(ManifestReaderCompatibilityTest, IteratorHelpersAdaptLegacyReaders) { +TEST(ManifestReaderCompatibilityTest, StreamHelpersAdaptLegacyReaders) { LegacyManifestReader reader; ManifestReader& base_reader = reader; - ICEBERG_UNWRAP_OR_FAIL(auto entries, base_reader.EntriesIterator()); + ICEBERG_UNWRAP_OR_FAIL(auto entries, base_reader.EntriesStream()); ICEBERG_UNWRAP_OR_FAIL(auto entry, entries->Next()); EXPECT_FALSE(entry.has_value()); EXPECT_EQ(reader.entries_calls, 1); - ICEBERG_UNWRAP_OR_FAIL(auto live_entries, base_reader.LiveEntriesIterator()); + ICEBERG_UNWRAP_OR_FAIL(auto live_entries, base_reader.LiveEntriesStream()); ICEBERG_UNWRAP_OR_FAIL(auto live_entry, live_entries->Next()); EXPECT_FALSE(live_entry.has_value()); EXPECT_EQ(reader.live_entries_calls, 1); @@ -250,7 +250,7 @@ TEST_P(TestManifestReader, TestManifestReaderWithEmptyInheritableMetadata) { EXPECT_EQ(read_entry.snapshot_id, 1000L); } -TEST_P(TestManifestReader, EntriesIteratorOwnsReaderResources) { +TEST_P(TestManifestReader, EntriesStreamOwnsReaderResources) { auto version = GetParam(); auto file_a = MakeDataFile("/path/to/data-a.parquet", PartitionValues({Literal::Int(0)})); @@ -266,7 +266,7 @@ TEST_P(TestManifestReader, EntriesIteratorOwnsReaderResources) { ICEBERG_UNWRAP_OR_FAIL(auto reader, ManifestReader::Make(manifest, file_io_, schema_, spec_)); - ICEBERG_UNWRAP_OR_FAIL(auto iterator, reader->EntriesIterator()); + ICEBERG_UNWRAP_OR_FAIL(auto iterator, reader->EntriesStream()); reader.reset(); ICEBERG_UNWRAP_OR_FAIL(auto first, iterator->Next()); diff --git a/src/iceberg/test/scan_planning_metrics_test.cc b/src/iceberg/test/scan_planning_metrics_test.cc index 38ea8796f..688db6e01 100644 --- a/src/iceberg/test/scan_planning_metrics_test.cc +++ b/src/iceberg/test/scan_planning_metrics_test.cc @@ -266,7 +266,7 @@ TEST_P(ScanPlanningMetricsTest, IteratorReportsWhenDestroyedEarly) { ICEBERG_UNWRAP_OR_FAIL(auto builder, MakeScanBuilder(metadata)); builder->ReportWith(reporter_); ICEBERG_UNWRAP_OR_FAIL(auto scan, builder->Build()); - ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesIterator()); + ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesStream()); scan.reset(); ICEBERG_UNWRAP_OR_FAIL(auto first, iterator->Next()); @@ -300,7 +300,7 @@ TEST_P(ScanPlanningMetricsTest, IteratorDoesNotReportFailedPlanning) { ICEBERG_UNWRAP_OR_FAIL(auto builder, MakeScanBuilder(metadata)); builder->ReportWith(reporter_); ICEBERG_UNWRAP_OR_FAIL(auto scan, builder->Build()); - ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesIterator()); + ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesStream()); auto next = iterator->Next(); EXPECT_FALSE(next.has_value()); diff --git a/src/iceberg/test/table_scan_test.cc b/src/iceberg/test/table_scan_test.cc index 5c07926fc..ba551893a 100644 --- a/src/iceberg/test/table_scan_test.cc +++ b/src/iceberg/test/table_scan_test.cc @@ -321,7 +321,7 @@ TEST_P(TableScanTest, DataTableScanPlanFilesEmpty) { ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); EXPECT_TRUE(tasks.empty()); - ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesIterator()); + ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesStream()); ICEBERG_UNWRAP_OR_FAIL(auto next, iterator->Next()); EXPECT_FALSE(next.has_value()); } @@ -385,7 +385,7 @@ TEST_P(TableScanTest, PlanFilesWithDataManifests) { EXPECT_THAT(GetPaths(tasks), testing::UnorderedElementsAre("/path/to/data1.parquet", "/path/to/data2.parquet")); - ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesIterator()); + ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesStream()); scan.reset(); ICEBERG_UNWRAP_OR_FAIL(auto streamed_tasks, iterator->ToVector()); ASSERT_EQ(streamed_tasks.size(), 2); @@ -695,7 +695,7 @@ TEST_P(TableScanTest, PlanFilesWithDeleteFiles) { }; verify_tasks(tasks); - ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesIterator()); + ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesStream()); ICEBERG_UNWRAP_OR_FAIL(auto streamed_tasks, iterator->ToVector()); verify_tasks(streamed_tasks); } diff --git a/src/iceberg/type_fwd.h b/src/iceberg/type_fwd.h index 83fcb8051..6abe6635e 100644 --- a/src/iceberg/type_fwd.h +++ b/src/iceberg/type_fwd.h @@ -197,7 +197,7 @@ class ManifestListReader; class ManifestListWriter; class ManifestReader; class ManifestWriter; -class SupportsManifestEntryIteration; +class SupportsManifestEntryStreaming; class PartitionSummary; /// \brief File I/O. From c88ab044b494d1e708f0a8d2f05d52f79503e864 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Thu, 10 Sep 2026 22:25:14 +0800 Subject: [PATCH 15/21] refactor: use stream terminology consistently Co-authored-by: Codex --- src/iceberg/file_scan_task_stream.h | 6 +- src/iceberg/manifest/manifest_group.cc | 73 +++++++++---------- src/iceberg/manifest/manifest_group.h | 2 +- src/iceberg/manifest/manifest_reader.cc | 48 ++++++------ src/iceberg/manifest/manifest_reader.h | 14 ++-- .../manifest/manifest_reader_internal.h | 8 +- src/iceberg/table_scan.cc | 32 ++++---- src/iceberg/test/CMakeLists.txt | 2 +- src/iceberg/test/manifest_group_test.cc | 10 +-- src/iceberg/test/manifest_reader_test.cc | 8 +- src/iceberg/test/meson.build | 2 +- .../test/scan_planning_metrics_test.cc | 16 ++-- .../test/{iterator_test.cc => stream_test.cc} | 72 +++++++++--------- src/iceberg/test/table_scan_test.cc | 12 +-- src/iceberg/type_fwd.h | 2 +- src/iceberg/util/meson.build | 2 +- src/iceberg/util/{iterator.h => stream.h} | 34 ++++----- 17 files changed, 171 insertions(+), 172 deletions(-) rename src/iceberg/test/{iterator_test.cc => stream_test.cc} (66%) rename src/iceberg/util/{iterator.h => stream.h} (77%) diff --git a/src/iceberg/file_scan_task_stream.h b/src/iceberg/file_scan_task_stream.h index 307c907b1..f88a8a671 100644 --- a/src/iceberg/file_scan_task_stream.h +++ b/src/iceberg/file_scan_task_stream.h @@ -3,7 +3,7 @@ * or more contributor license agreements. See the NOTICE file * distributed with this work for additional information * regarding copyright ownership. The ASF licenses this file - * under the Apache License, Version 2.0 (the + * to you under the Apache License, Version 2.0 (the * "License"); you may not use this file except in compliance * with the License. You may obtain a copy of the License at * @@ -24,12 +24,12 @@ #include -#include "iceberg/util/iterator.h" +#include "iceberg/util/stream.h" namespace iceberg { class FileScanTask; -using FileScanTaskStream = std::unique_ptr>>; +using FileScanTaskStream = std::unique_ptr>>; } // namespace iceberg diff --git a/src/iceberg/manifest/manifest_group.cc b/src/iceberg/manifest/manifest_group.cc index 7af5315f1..83fb349c6 100644 --- a/src/iceberg/manifest/manifest_group.cc +++ b/src/iceberg/manifest/manifest_group.cc @@ -131,8 +131,8 @@ ManifestGroup::~ManifestGroup() = default; ManifestGroup::ManifestGroup(ManifestGroup&&) noexcept = default; ManifestGroup& ManifestGroup::operator=(ManifestGroup&&) noexcept = default; -class ManifestGroup::FilePlanningIterator final - : public Iterator> { +class ManifestGroup::FilePlanningStream final + : public Stream> { public: static Result Make(std::unique_ptr group) { ICEBERG_RETURN_UNEXPECTED(group->CheckErrors()); @@ -153,7 +153,7 @@ class ManifestGroup::FilePlanningIterator final } const bool drop_stats = stats_projection.drop_stats; - return FileScanTaskStream(new FilePlanningIterator( + return FileScanTaskStream(new FilePlanningStream( std::move(group), std::move(delete_index), std::move(data_file_evaluator), std::move(stats_projection.columns), drop_stats)); } @@ -210,10 +210,10 @@ class ManifestGroup::FilePlanningIterator final } private: - FilePlanningIterator(std::unique_ptr group, - std::unique_ptr delete_index, - std::unique_ptr data_file_evaluator, - std::vector columns, bool drop_stats) + FilePlanningStream(std::unique_ptr group, + std::unique_ptr delete_index, + std::unique_ptr data_file_evaluator, + std::vector columns, bool drop_stats) : group_(std::move(group)), delete_index_(std::move(delete_index)), data_file_evaluator_(std::move(data_file_evaluator)), @@ -221,21 +221,21 @@ class ManifestGroup::FilePlanningIterator final drop_stats_(drop_stats) {} using TaggedEntry = std::pair; - using TaggedIterator = std::pair>>; + using TaggedStream = std::pair>>; Result> NextEntry() { if (!group_->executor_.has_value()) { while (true) { - if (!entry_iterator_) { + if (!entry_stream_) { ICEBERG_ASSIGN_OR_RAISE(bool opened, OpenNextManifest()); if (!opened) { return std::nullopt; } } - ICEBERG_ASSIGN_OR_RAISE(auto entry, entry_iterator_->Next()); + ICEBERG_ASSIGN_OR_RAISE(auto entry, entry_stream_->Next()); if (!entry.has_value()) { - entry_iterator_.reset(); + entry_stream_.reset(); continue; } return std::optional{std::in_place, current_spec_id_, @@ -244,18 +244,18 @@ class ManifestGroup::FilePlanningIterator final } while (true) { - if (next_batch_iterator_ == batch_iterators_.size()) { + if (next_batch_stream_ == batch_streams_.size()) { ICEBERG_ASSIGN_OR_RAISE(bool loaded, LoadNextManifestBatch()); if (!loaded) { return std::nullopt; } } - auto& [spec_id, iterator] = batch_iterators_[next_batch_iterator_]; - ICEBERG_ASSIGN_OR_RAISE(auto entry, iterator->Next()); + auto& [spec_id, stream] = batch_streams_[next_batch_stream_]; + ICEBERG_ASSIGN_OR_RAISE(auto entry, stream->Next()); if (!entry.has_value()) { - iterator.reset(); - ++next_batch_iterator_; + stream.reset(); + ++next_batch_stream_; continue; } return std::optional{std::in_place, spec_id, std::move(entry).value()}; @@ -339,9 +339,9 @@ class ManifestGroup::FilePlanningIterator final } ICEBERG_ASSIGN_OR_RAISE(auto reader, group_->MakeReader(manifest, columns_)); - ICEBERG_ASSIGN_OR_RAISE(entry_iterator_, group_->ignore_deleted_ - ? reader->LiveEntriesStream() - : reader->EntriesStream()); + ICEBERG_ASSIGN_OR_RAISE(entry_stream_, group_->ignore_deleted_ + ? reader->LiveEntriesStream() + : reader->EntriesStream()); current_spec_id_ = manifest.partition_spec_id; return true; } @@ -364,26 +364,25 @@ class ManifestGroup::FilePlanningIterator final return false; } - // Open the readers concurrently, but keep their iterators instead of collecting + // Open the readers concurrently, but keep their streams instead of collecting // entries here. This preserves bounded memory for large manifests while retaining // parallel manifest initialization when an executor is configured. ICEBERG_ASSIGN_OR_RAISE( - batch_iterators_, + batch_streams_, ParallelCollect( group_->executor_, manifests, - [this](const ManifestFile* manifest) -> Result> { + [this](const ManifestFile* manifest) -> Result> { ICEBERG_ASSIGN_OR_RAISE(auto reader, group_->MakeReader(*manifest, columns_)); - ICEBERG_ASSIGN_OR_RAISE(auto iterator, group_->ignore_deleted_ - ? reader->LiveEntriesStream() - : reader->EntriesStream()); - - std::vector tagged_iterators; - tagged_iterators.emplace_back(manifest->partition_spec_id, - std::move(iterator)); - return tagged_iterators; + ICEBERG_ASSIGN_OR_RAISE(auto stream, group_->ignore_deleted_ + ? reader->LiveEntriesStream() + : reader->EntriesStream()); + + std::vector tagged_streams; + tagged_streams.emplace_back(manifest->partition_spec_id, std::move(stream)); + return tagged_streams; })); - next_batch_iterator_ = 0; + next_batch_stream_ = 0; return true; } @@ -423,14 +422,14 @@ class ManifestGroup::FilePlanningIterator final std::vector columns_; std::unordered_map> manifest_evaluators_; std::unordered_map> residual_evaluators_; - std::unique_ptr> entry_iterator_; - std::vector batch_iterators_; + std::unique_ptr> entry_stream_; + std::vector batch_streams_; size_t next_manifest_ = 0; - size_t next_batch_iterator_ = 0; + size_t next_batch_stream_ = 0; int32_t current_spec_id_ = 0; bool drop_stats_; - // Limit the number of manifest readers and iterators retained by executor-backed + // Limit the number of manifest readers and streams retained by executor-backed // planning. The executor still controls actual task concurrency, while this fixed // cap prevents resource use from scaling with the total manifest count. Entries // within each manifest remain streamed, so this does not cap manifest size. @@ -562,7 +561,7 @@ Result>> ManifestGroup::PlanFiles() { Result ManifestGroup::PlanFilesStream() && { auto group = std::make_unique(std::move(*this)); - return FilePlanningIterator::Make(std::move(group)); + return FilePlanningStream::Make(std::move(group)); } Result>> ManifestGroup::Plan( @@ -691,7 +690,7 @@ ManifestGroup::StatsProjection ManifestGroup::PrepareStatsProjection( // The caller's projection records whether stats were requested. Equality-delete // matching may add stats temporarily, but they should still be dropped from the // result when the original projection did not request them. Keeping this decision - // here ensures eager and iterator planning use identical semantics. + // here ensures eager and stream planning use identical semantics. StatsProjection result{.columns = columns_, .drop_stats = ManifestReader::ShouldDropStats(columns_)}; if (has_equality_deletes) { diff --git a/src/iceberg/manifest/manifest_group.h b/src/iceberg/manifest/manifest_group.h index f0cb5e360..36ce7814a 100644 --- a/src/iceberg/manifest/manifest_group.h +++ b/src/iceberg/manifest/manifest_group.h @@ -164,7 +164,7 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector { const CreateTasksFunction& create_tasks); private: - class FilePlanningIterator; + class FilePlanningStream; struct StatsProjection { std::vector columns; diff --git a/src/iceberg/manifest/manifest_reader.cc b/src/iceberg/manifest/manifest_reader.cc index f746d2904..66b18d35b 100644 --- a/src/iceberg/manifest/manifest_reader.cc +++ b/src/iceberg/manifest/manifest_reader.cc @@ -691,9 +691,9 @@ Result> ProjectSchema(std::shared_ptr schema, } template -class VectorIterator final : public Iterator { +class VectorStream final : public Stream { public: - explicit VectorIterator(std::vector values) : values_(std::move(values)) {} + explicit VectorStream(std::vector values) : values_(std::move(values)) {} Result> NextImpl() override { if (next_ == values_.size()) { @@ -707,16 +707,16 @@ class VectorIterator final : public Iterator { size_t next_ = 0; }; -class ManifestEntryIteratorImpl final : public Iterator { +class ManifestEntryStreamImpl final : public Stream { public: - ManifestEntryIteratorImpl(std::unique_ptr reader, - std::shared_ptr file_schema, ArrowSchema arrow_schema, - std::shared_ptr inheritable_metadata, - std::optional first_row_id, bool is_committed, - bool only_live, std::unique_ptr evaluator, - std::unique_ptr metrics_evaluator, - std::shared_ptr partition_set, - std::shared_ptr skip_counter, bool drop_stats) + ManifestEntryStreamImpl(std::unique_ptr reader, + std::shared_ptr file_schema, ArrowSchema arrow_schema, + std::shared_ptr inheritable_metadata, + std::optional first_row_id, bool is_committed, + bool only_live, std::unique_ptr evaluator, + std::unique_ptr metrics_evaluator, + std::shared_ptr partition_set, + std::shared_ptr skip_counter, bool drop_stats) : reader_(std::move(reader)), file_schema_(std::move(file_schema)), arrow_schema_(std::exchange(arrow_schema, ArrowSchema{})), @@ -815,20 +815,20 @@ class ManifestEntryIteratorImpl final : public Iterator { } // namespace -Result>> ManifestReader::EntriesStream() { - if (auto* iterable = dynamic_cast(this)) { - return iterable->EntriesStream(); +Result>> ManifestReader::EntriesStream() { + if (auto* streaming_reader = dynamic_cast(this)) { + return streaming_reader->EntriesStream(); } ICEBERG_ASSIGN_OR_RAISE(auto entries, Entries()); - return std::make_unique>(std::move(entries)); + return std::make_unique>(std::move(entries)); } -Result>> ManifestReader::LiveEntriesStream() { - if (auto* iterable = dynamic_cast(this)) { - return iterable->LiveEntriesStream(); +Result>> ManifestReader::LiveEntriesStream() { + if (auto* streaming_reader = dynamic_cast(this)) { + return streaming_reader->LiveEntriesStream(); } ICEBERG_ASSIGN_OR_RAISE(auto entries, LiveEntries()); - return std::make_unique>(std::move(entries)); + return std::make_unique>(std::move(entries)); } bool ManifestReader::ShouldDropStats(const std::vector& columns) { @@ -1011,15 +1011,15 @@ Result> ManifestReaderImpl::LiveEntries() { return entries->ToVector(); } -Result>> ManifestReaderImpl::EntriesStream() { +Result>> ManifestReaderImpl::EntriesStream() { return MakeEntriesStream(/*only_live=*/false); } -Result>> ManifestReaderImpl::LiveEntriesStream() { +Result>> ManifestReaderImpl::LiveEntriesStream() { return MakeEntriesStream(/*only_live=*/true); } -Result>> ManifestReaderImpl::MakeEntriesStream( +Result>> ManifestReaderImpl::MakeEntriesStream( bool only_live) { ICEBERG_ASSIGN_OR_RAISE(auto partition_type, spec_->RawPartitionType(*schema_)); auto data_file_schema = DataFile::Type(std::move(partition_type))->ToSchema(); @@ -1060,13 +1060,13 @@ Result>> ManifestReaderImpl::MakeEntries } bool drop_stats = drop_stats_ && ShouldDropStats(columns_); - auto iterator = std::unique_ptr>(new ManifestEntryIteratorImpl( + auto stream = std::unique_ptr>(new ManifestEntryStreamImpl( std::move(file_reader_), file_schema_, std::move(arrow_schema), inheritable_metadata_, first_row_id_, is_committed_, only_live, std::move(evaluator), std::move(metrics_evaluator), partition_set_, skip_counter_, drop_stats)); schema_guard.Release(); - return iterator; + return stream; } Result> ManifestListReaderImpl::Files() const { diff --git a/src/iceberg/manifest/manifest_reader.h b/src/iceberg/manifest/manifest_reader.h index 5d16795f7..61b8e41eb 100644 --- a/src/iceberg/manifest/manifest_reader.h +++ b/src/iceberg/manifest/manifest_reader.h @@ -33,7 +33,7 @@ #include "iceberg/metrics/counter.h" #include "iceberg/result.h" #include "iceberg/type_fwd.h" -#include "iceberg/util/iterator.h" +#include "iceberg/util/stream.h" namespace iceberg { @@ -53,14 +53,14 @@ class ICEBERG_EXPORT ManifestReader { /// Implementations using SupportsManifestEntryStreaming produce entries lazily. Other /// implementations are adapted from Entries() for compatibility. The returned stream /// is fallible and single-pass. - Result>> EntriesStream(); + Result>> EntriesStream(); /// \brief Lazily read only live (non-deleted) manifest entries. /// /// Implementations using SupportsManifestEntryStreaming produce entries lazily. Other /// implementations are adapted from LiveEntries() for compatibility. The returned /// stream is fallible and single-pass. - Result>> LiveEntriesStream(); + Result>> LiveEntriesStream(); /// \brief Select specific columns of data file to read from the manifest entries. /// @@ -155,15 +155,15 @@ class ICEBERG_EXPORT SupportsManifestEntryStreaming { /// \brief Lazily read manifest entries. /// - /// The returned stream must own all resources required for iteration and must not + /// The returned stream must own all resources required for consumption and must not /// depend on this reader remaining alive. - virtual Result>> EntriesStream() = 0; + virtual Result>> EntriesStream() = 0; /// \brief Lazily read only live (non-deleted) manifest entries. /// - /// The returned stream must own all resources required for iteration and must not + /// The returned stream must own all resources required for consumption and must not /// depend on this reader remaining alive. - virtual Result>> LiveEntriesStream() = 0; + virtual Result>> LiveEntriesStream() = 0; }; /// \brief Read manifest files from a manifest list file. diff --git a/src/iceberg/manifest/manifest_reader_internal.h b/src/iceberg/manifest/manifest_reader_internal.h index f6c4b1301..9f576ff18 100644 --- a/src/iceberg/manifest/manifest_reader_internal.h +++ b/src/iceberg/manifest/manifest_reader_internal.h @@ -66,9 +66,9 @@ class ManifestReaderImpl : public ManifestReader, public SupportsManifestEntrySt Result> LiveEntries() override; - Result>> EntriesStream() override; + Result>> EntriesStream() override; - Result>> LiveEntriesStream() override; + Result>> LiveEntriesStream() override; ManifestReader& Select(const std::vector& columns) override; @@ -85,8 +85,8 @@ class ManifestReaderImpl : public ManifestReader, public SupportsManifestEntrySt ManifestReader& SkipCounter(std::shared_ptr counter) override; private: - /// \brief Create an entry iterator with optional live-only filtering. - Result>> MakeEntriesStream(bool only_live); + /// \brief Create an entry stream with optional live-only filtering. + Result>> MakeEntriesStream(bool only_live); /// \brief Lazily open the underlying Avro reader with appropriate schema projection. Status OpenReader(std::shared_ptr projection); diff --git a/src/iceberg/table_scan.cc b/src/iceberg/table_scan.cc index 2bcaad44b..cefb000bd 100644 --- a/src/iceberg/table_scan.cc +++ b/src/iceberg/table_scan.cc @@ -67,7 +67,7 @@ const std::vector kScanColumnsWithStats = [] { }(); template -class EmptyIterator final : public Iterator { +class EmptyStream final : public Stream { public: Result> NextImpl() override { return std::nullopt; } }; @@ -108,23 +108,23 @@ Result MakeScanReport(const DataTableScan& scan, const Snapshot& sna }; } -class ReportingFileTaskIterator final : public Iterator> { +class ReportingFileTaskStream final : public Stream> { public: - ReportingFileTaskIterator(FileScanTaskStream iterator, - std::shared_ptr scan_metrics, - std::chrono::nanoseconds planning_duration, - std::shared_ptr reporter, ScanReport report) - : iterator_(std::move(iterator)), + ReportingFileTaskStream(FileScanTaskStream stream, + std::shared_ptr scan_metrics, + std::chrono::nanoseconds planning_duration, + std::shared_ptr reporter, ScanReport report) + : stream_(std::move(stream)), scan_metrics_(std::move(scan_metrics)), planning_duration_(std::move(planning_duration)), reporter_(std::move(reporter)), report_(std::move(report)) {} - ~ReportingFileTaskIterator() override { Finalize(); } + ~ReportingFileTaskStream() override { Finalize(); } Result>> NextImpl() override { auto start = std::chrono::steady_clock::now(); - auto result = iterator_->Next(); + auto result = stream_->Next(); planning_duration_ += std::chrono::duration_cast( std::chrono::steady_clock::now() - start); if (!result.has_value()) { @@ -147,7 +147,7 @@ class ReportingFileTaskIterator final : public IteratorReport(report_); } - FileScanTaskStream iterator_; + FileScanTaskStream stream_; std::shared_ptr scan_metrics_; std::chrono::nanoseconds planning_duration_; std::shared_ptr reporter_; @@ -724,7 +724,7 @@ Result>> DataTableScan::PlanFiles() co Result DataTableScan::PlanFilesStream() const { ICEBERG_ASSIGN_OR_RAISE(auto snapshot, this->snapshot()); if (!snapshot) { - return std::make_unique>>(); + return std::make_unique>>(); } std::shared_ptr scan_metrics; @@ -771,9 +771,9 @@ Result DataTableScan::PlanFilesStream() const { manifest_group->IgnoreResiduals(); } - ICEBERG_ASSIGN_OR_RAISE(auto iterator, std::move(*manifest_group).PlanFilesStream()); + ICEBERG_ASSIGN_OR_RAISE(auto stream, std::move(*manifest_group).PlanFilesStream()); if (!planning_start.has_value()) { - return iterator; + return stream; } auto planning_duration = std::chrono::duration_cast( @@ -782,11 +782,11 @@ Result DataTableScan::PlanFilesStream() const { auto report = MakeScanReport(*this, *snapshot, ScanMetricsResult{}); if (!report.has_value()) { // Scan reporting is best effort, matching PlanFiles(). - return iterator; + return stream; } - return std::make_unique( - std::move(iterator), std::move(scan_metrics), planning_duration, + return std::make_unique( + std::move(stream), std::move(scan_metrics), planning_duration, context_.metrics_reporter, std::move(report).value()); } diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index 5ca9fd915..4f9767496 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -135,7 +135,7 @@ add_iceberg_test(util_test endian_test.cc file_io_test.cc formatter_test.cc - iterator_test.cc + stream_test.cc lazy_test.cc location_util_test.cc math_util_internal_test.cc diff --git a/src/iceberg/test/manifest_group_test.cc b/src/iceberg/test/manifest_group_test.cc index d6b742f44..f1df2d09e 100644 --- a/src/iceberg/test/manifest_group_test.cc +++ b/src/iceberg/test/manifest_group_test.cc @@ -323,8 +323,8 @@ TEST_P(ManifestGroupTest, PlanFilesStreamPreservesSelectAllWithEqualityDeletes) ICEBERG_UNWRAP_OR_FAIL( auto group, ManifestGroup::Make(file_io_, schema_, GetSpecsById(), {data_manifest}, {delete_manifest})); - ICEBERG_UNWRAP_OR_FAIL(auto iterator, std::move(*group).PlanFilesStream()); - ICEBERG_UNWRAP_OR_FAIL(auto task, iterator->Next()); + ICEBERG_UNWRAP_OR_FAIL(auto stream, std::move(*group).PlanFilesStream()); + ICEBERG_UNWRAP_OR_FAIL(auto task, stream->Next()); ASSERT_TRUE(task.has_value()); EXPECT_EQ(task.value()->data_file()->file_path, "/path/to/data.parquet"); @@ -335,7 +335,7 @@ TEST_P(ManifestGroupTest, PlanFilesStreamPreservesSelectAllWithEqualityDeletes) EXPECT_EQ(task.value()->delete_files().front()->file_path, "/path/to/equality-delete.parquet"); - ICEBERG_UNWRAP_OR_FAIL(auto end, iterator->Next()); + ICEBERG_UNWRAP_OR_FAIL(auto end, stream->Next()); EXPECT_FALSE(end.has_value()); } @@ -753,8 +753,8 @@ TEST_P(ManifestGroupTest, PlanFilesStreamUsesExecutor) { test::ThreadExecutor executor; group->PlanWith(std::ref(executor)); - ICEBERG_UNWRAP_OR_FAIL(auto iterator, std::move(*group).PlanFilesStream()); - ICEBERG_UNWRAP_OR_FAIL(auto tasks, iterator->ToVector()); + ICEBERG_UNWRAP_OR_FAIL(auto stream, std::move(*group).PlanFilesStream()); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, stream->ToVector()); EXPECT_THAT(GetPaths(tasks), testing::UnorderedElementsAre("/path/to/data1.parquet", "/path/to/data2.parquet")); diff --git a/src/iceberg/test/manifest_reader_test.cc b/src/iceberg/test/manifest_reader_test.cc index a2c30be4e..ceec53d62 100644 --- a/src/iceberg/test/manifest_reader_test.cc +++ b/src/iceberg/test/manifest_reader_test.cc @@ -266,18 +266,18 @@ TEST_P(TestManifestReader, EntriesStreamOwnsReaderResources) { ICEBERG_UNWRAP_OR_FAIL(auto reader, ManifestReader::Make(manifest, file_io_, schema_, spec_)); - ICEBERG_UNWRAP_OR_FAIL(auto iterator, reader->EntriesStream()); + ICEBERG_UNWRAP_OR_FAIL(auto stream, reader->EntriesStream()); reader.reset(); - ICEBERG_UNWRAP_OR_FAIL(auto first, iterator->Next()); + ICEBERG_UNWRAP_OR_FAIL(auto first, stream->Next()); ASSERT_TRUE(first.has_value()); EXPECT_EQ(first->data_file->file_path, "/path/to/data-a.parquet"); - ICEBERG_UNWRAP_OR_FAIL(auto second, iterator->Next()); + ICEBERG_UNWRAP_OR_FAIL(auto second, stream->Next()); ASSERT_TRUE(second.has_value()); EXPECT_EQ(second->data_file->file_path, "/path/to/data-b.parquet"); - ICEBERG_UNWRAP_OR_FAIL(auto end, iterator->Next()); + ICEBERG_UNWRAP_OR_FAIL(auto end, stream->Next()); EXPECT_FALSE(end.has_value()); } diff --git a/src/iceberg/test/meson.build b/src/iceberg/test/meson.build index 6844df5e9..4f00bdff5 100644 --- a/src/iceberg/test/meson.build +++ b/src/iceberg/test/meson.build @@ -110,7 +110,7 @@ iceberg_tests = { 'executor_util_test.cc', 'file_io_test.cc', 'formatter_test.cc', - 'iterator_test.cc', + 'stream_test.cc', 'lazy_test.cc', 'location_util_test.cc', 'math_util_internal_test.cc', diff --git a/src/iceberg/test/scan_planning_metrics_test.cc b/src/iceberg/test/scan_planning_metrics_test.cc index 688db6e01..215935b3a 100644 --- a/src/iceberg/test/scan_planning_metrics_test.cc +++ b/src/iceberg/test/scan_planning_metrics_test.cc @@ -244,7 +244,7 @@ TEST_P(ScanPlanningMetricsTest, ReportsToTableAndScanReporters) { EXPECT_EQ(scan_reporter->last()->table_name, "test.table"); } -TEST_P(ScanPlanningMetricsTest, IteratorReportsWhenDestroyedEarly) { +TEST_P(ScanPlanningMetricsTest, StreamReportsWhenDestroyedEarly) { auto version = GetParam(); constexpr int64_t kSnapshotId = 2010L; const auto part = PartitionValues({Literal::Int(0)}); @@ -266,14 +266,14 @@ TEST_P(ScanPlanningMetricsTest, IteratorReportsWhenDestroyedEarly) { ICEBERG_UNWRAP_OR_FAIL(auto builder, MakeScanBuilder(metadata)); builder->ReportWith(reporter_); ICEBERG_UNWRAP_OR_FAIL(auto scan, builder->Build()); - ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesStream()); + ICEBERG_UNWRAP_OR_FAIL(auto stream, scan->PlanFilesStream()); scan.reset(); - ICEBERG_UNWRAP_OR_FAIL(auto first, iterator->Next()); + ICEBERG_UNWRAP_OR_FAIL(auto first, stream->Next()); ASSERT_TRUE(first.has_value()); EXPECT_EQ(reporter_->report_count(), 0); - iterator.reset(); + stream.reset(); ASSERT_EQ(reporter_->report_count(), 1); ASSERT_TRUE(reporter_->last().has_value()); const auto& metrics = reporter_->last()->scan_metrics; @@ -281,7 +281,7 @@ TEST_P(ScanPlanningMetricsTest, IteratorReportsWhenDestroyedEarly) { EXPECT_EQ(metrics.result_data_files->value, 1); } -TEST_P(ScanPlanningMetricsTest, IteratorDoesNotReportFailedPlanning) { +TEST_P(ScanPlanningMetricsTest, StreamDoesNotReportFailedPlanning) { auto version = GetParam(); constexpr int64_t kSnapshotId = 2011L; const auto part = PartitionValues({Literal::Int(0)}); @@ -300,13 +300,13 @@ TEST_P(ScanPlanningMetricsTest, IteratorDoesNotReportFailedPlanning) { ICEBERG_UNWRAP_OR_FAIL(auto builder, MakeScanBuilder(metadata)); builder->ReportWith(reporter_); ICEBERG_UNWRAP_OR_FAIL(auto scan, builder->Build()); - ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesStream()); + ICEBERG_UNWRAP_OR_FAIL(auto stream, scan->PlanFilesStream()); - auto next = iterator->Next(); + auto next = stream->Next(); EXPECT_FALSE(next.has_value()); EXPECT_EQ(reporter_->report_count(), 0); - iterator.reset(); + stream.reset(); EXPECT_EQ(reporter_->report_count(), 0); } diff --git a/src/iceberg/test/iterator_test.cc b/src/iceberg/test/stream_test.cc similarity index 66% rename from src/iceberg/test/iterator_test.cc rename to src/iceberg/test/stream_test.cc index ffc21bc60..ebb126e60 100644 --- a/src/iceberg/test/iterator_test.cc +++ b/src/iceberg/test/stream_test.cc @@ -49,7 +49,7 @@ static_assert(std::is_copy_constructible_v); static_assert(!std::is_move_constructible_v); // Exercises ToVector() with values that can be copied but not moved. -class CopyOnlyIterator final : public Iterator { +class CopyOnlyStream final : public Stream { private: Result> NextImpl() override { if (next_ == 3) { @@ -62,7 +62,7 @@ class CopyOnlyIterator final : public Iterator { }; // Exercises ToVector() with values that can be moved but not copied. -class MoveOnlyIterator final : public Iterator> { +class MoveOnlyStream final : public Stream> { public: int calls() const { return calls_; } @@ -81,7 +81,7 @@ class MoveOnlyIterator final : public Iterator> { }; // Exercises ToVector() error propagation after some values have been consumed. -class FailingIterator final : public Iterator { +class FailingStream final : public Stream { public: int calls() const { return calls_; } @@ -91,27 +91,27 @@ class FailingIterator final : public Iterator { if (next_ < 2) { return Result>(std::in_place, std::in_place, next_++); } - return Invalid("iteration failed"); + return Invalid("stream failed"); } int next_ = 0; int calls_ = 0; }; -static_assert(std::is_move_constructible_v); -static_assert(std::is_move_assignable_v); -static_assert(std::is_move_constructible_v); -static_assert(std::is_move_assignable_v); +static_assert(std::is_move_constructible_v); +static_assert(std::is_move_assignable_v); +static_assert(std::is_move_constructible_v); +static_assert(std::is_move_assignable_v); -TEST(IteratorTest, FileScanTaskStreamSupportsIncompleteFileScanTask) { - FileScanTaskStream iterator; - EXPECT_EQ(iterator, nullptr); +TEST(StreamTest, FileScanTaskStreamSupportsIncompleteFileScanTask) { + FileScanTaskStream stream; + EXPECT_EQ(stream, nullptr); } -TEST(IteratorTest, ToVectorSupportsCopyOnlyValues) { - CopyOnlyIterator iterator; +TEST(StreamTest, ToVectorSupportsCopyOnlyValues) { + CopyOnlyStream stream; - ICEBERG_UNWRAP_OR_FAIL(auto values, iterator.ToVector()); + ICEBERG_UNWRAP_OR_FAIL(auto values, stream.ToVector()); ASSERT_EQ(values.size(), 3); EXPECT_EQ(values[0].value(), 0); @@ -119,10 +119,10 @@ TEST(IteratorTest, ToVectorSupportsCopyOnlyValues) { EXPECT_EQ(values[2].value(), 2); } -TEST(IteratorTest, ToVectorSupportsMoveOnlyValues) { - MoveOnlyIterator iterator; +TEST(StreamTest, ToVectorSupportsMoveOnlyValues) { + MoveOnlyStream stream; - ICEBERG_UNWRAP_OR_FAIL(auto values, iterator.ToVector()); + ICEBERG_UNWRAP_OR_FAIL(auto values, stream.ToVector()); ASSERT_EQ(values.size(), 3); EXPECT_EQ(*values[0], 0); @@ -130,46 +130,46 @@ TEST(IteratorTest, ToVectorSupportsMoveOnlyValues) { EXPECT_EQ(*values[2], 2); } -TEST(IteratorTest, NextRemainsAtEndAfterExhaustion) { - MoveOnlyIterator iterator; - ICEBERG_UNWRAP_OR_FAIL(auto values, iterator.ToVector()); +TEST(StreamTest, NextRemainsAtEndAfterExhaustion) { + MoveOnlyStream stream; + ICEBERG_UNWRAP_OR_FAIL(auto values, stream.ToVector()); ASSERT_EQ(values.size(), 3); - EXPECT_EQ(iterator.calls(), 4); + EXPECT_EQ(stream.calls(), 4); for (int i = 0; i < 2; ++i) { - auto result = iterator.Next(); + auto result = stream.Next(); ASSERT_TRUE(result.has_value()); EXPECT_FALSE(result->has_value()); } - EXPECT_EQ(iterator.calls(), 4); + EXPECT_EQ(stream.calls(), 4); } -TEST(IteratorTest, ToVectorPropagatesErrorsAfterPartialConsumption) { - FailingIterator iterator; +TEST(StreamTest, ToVectorPropagatesErrorsAfterPartialConsumption) { + FailingStream stream; - ICEBERG_UNWRAP_OR_FAIL(auto first, iterator.Next()); + ICEBERG_UNWRAP_OR_FAIL(auto first, stream.Next()); ASSERT_TRUE(first.has_value()); EXPECT_EQ(first.value(), 0); - auto result = iterator.ToVector(); + auto result = stream.ToVector(); EXPECT_THAT(result, IsError(ErrorKind::kInvalid)); - EXPECT_THAT(result, HasErrorMessage("iteration failed")); + EXPECT_THAT(result, HasErrorMessage("stream failed")); } -TEST(IteratorTest, NextRepeatsErrorWithoutAdvancing) { - FailingIterator iterator; - auto first_error = iterator.ToVector(); +TEST(StreamTest, NextRepeatsErrorWithoutAdvancing) { + FailingStream stream; + auto first_error = stream.ToVector(); EXPECT_THAT(first_error, IsError(ErrorKind::kInvalid)); - EXPECT_THAT(first_error, HasErrorMessage("iteration failed")); - EXPECT_EQ(iterator.calls(), 3); + EXPECT_THAT(first_error, HasErrorMessage("stream failed")); + EXPECT_EQ(stream.calls(), 3); for (int i = 0; i < 2; ++i) { - auto result = iterator.Next(); + auto result = stream.Next(); EXPECT_THAT(result, IsError(ErrorKind::kInvalid)); - EXPECT_THAT(result, HasErrorMessage("iteration failed")); + EXPECT_THAT(result, HasErrorMessage("stream failed")); } - EXPECT_EQ(iterator.calls(), 3); + EXPECT_EQ(stream.calls(), 3); } } // namespace diff --git a/src/iceberg/test/table_scan_test.cc b/src/iceberg/test/table_scan_test.cc index ba551893a..ee447db06 100644 --- a/src/iceberg/test/table_scan_test.cc +++ b/src/iceberg/test/table_scan_test.cc @@ -321,8 +321,8 @@ TEST_P(TableScanTest, DataTableScanPlanFilesEmpty) { ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); EXPECT_TRUE(tasks.empty()); - ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesStream()); - ICEBERG_UNWRAP_OR_FAIL(auto next, iterator->Next()); + ICEBERG_UNWRAP_OR_FAIL(auto stream, scan->PlanFilesStream()); + ICEBERG_UNWRAP_OR_FAIL(auto next, stream->Next()); EXPECT_FALSE(next.has_value()); } @@ -385,9 +385,9 @@ TEST_P(TableScanTest, PlanFilesWithDataManifests) { EXPECT_THAT(GetPaths(tasks), testing::UnorderedElementsAre("/path/to/data1.parquet", "/path/to/data2.parquet")); - ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesStream()); + ICEBERG_UNWRAP_OR_FAIL(auto stream, scan->PlanFilesStream()); scan.reset(); - ICEBERG_UNWRAP_OR_FAIL(auto streamed_tasks, iterator->ToVector()); + ICEBERG_UNWRAP_OR_FAIL(auto streamed_tasks, stream->ToVector()); ASSERT_EQ(streamed_tasks.size(), 2); EXPECT_THAT( GetPaths(streamed_tasks), @@ -695,8 +695,8 @@ TEST_P(TableScanTest, PlanFilesWithDeleteFiles) { }; verify_tasks(tasks); - ICEBERG_UNWRAP_OR_FAIL(auto iterator, scan->PlanFilesStream()); - ICEBERG_UNWRAP_OR_FAIL(auto streamed_tasks, iterator->ToVector()); + ICEBERG_UNWRAP_OR_FAIL(auto stream, scan->PlanFilesStream()); + ICEBERG_UNWRAP_OR_FAIL(auto streamed_tasks, stream->ToVector()); verify_tasks(streamed_tasks); } diff --git a/src/iceberg/type_fwd.h b/src/iceberg/type_fwd.h index 6abe6635e..8226cefcb 100644 --- a/src/iceberg/type_fwd.h +++ b/src/iceberg/type_fwd.h @@ -231,7 +231,7 @@ struct SessionContext; /// \brief Task execution. class Executor; template -class Iterator; +class Stream; /// \brief Metrics reporting. class MetricsReporter; diff --git a/src/iceberg/util/meson.build b/src/iceberg/util/meson.build index 831cc5888..c819bbfe1 100644 --- a/src/iceberg/util/meson.build +++ b/src/iceberg/util/meson.build @@ -32,7 +32,7 @@ install_headers( 'formatter.h', 'functional.h', 'int128.h', - 'iterator.h', + 'stream.h', 'lazy.h', 'location_util.h', 'macros.h', diff --git a/src/iceberg/util/iterator.h b/src/iceberg/util/stream.h similarity index 77% rename from src/iceberg/util/iterator.h rename to src/iceberg/util/stream.h index 16717499f..d92d7cbde 100644 --- a/src/iceberg/util/iterator.h +++ b/src/iceberg/util/stream.h @@ -19,8 +19,8 @@ #pragma once -/// \file iceberg/util/iterator.h -/// \brief Pull-based iterator interface for fallible, lazily produced values. +/// \file iceberg/util/stream.h +/// \brief Pull-based stream interface for fallible, lazily produced values. #include #include @@ -32,27 +32,27 @@ namespace iceberg { -/// \brief A pull-based iterator whose reads may fail. +/// \brief A pull-based stream whose reads may fail. /// -/// Iterator implementations own any resources needed to produce values. Destroying an -/// iterator releases those resources, including when iteration stops before reaching the -/// end. Iterators are not thread-safe unless an implementation explicitly says otherwise. -/// Once Next() returns an error or std::nullopt, the iterator is terminal. Subsequent +/// Stream implementations own any resources needed to produce values. Destroying a +/// stream releases those resources, including when consumption stops before reaching the +/// end. Streams are not thread-safe unless an implementation explicitly says otherwise. +/// Once Next() returns an error or std::nullopt, the stream is terminal. Subsequent /// calls return the same terminal result without invoking the implementation again. /// -/// \tparam T Value returned by the iterator. +/// \tparam T Value returned by the stream. template -class Iterator { +class Stream { public: - virtual ~Iterator() = default; + virtual ~Stream() = default; - Iterator() = default; - Iterator(const Iterator&) = delete; - Iterator& operator=(const Iterator&) = delete; - Iterator(Iterator&&) noexcept = default; - Iterator& operator=(Iterator&&) noexcept = default; + Stream() = default; + Stream(const Stream&) = delete; + Stream& operator=(const Stream&) = delete; + Stream(Stream&&) noexcept = default; + Stream& operator=(Stream&&) noexcept = default; - /// \brief Return the next value, or std::nullopt when the iterator is exhausted. + /// \brief Return the next value, or std::nullopt when the stream is exhausted. /// /// After this method returns an error or std::nullopt, subsequent calls return the same /// terminal result without invoking NextImpl(). @@ -92,7 +92,7 @@ class Iterator { if constexpr (!std::is_move_constructible_v) { static_assert(std::is_copy_constructible_v, - "Iterator::ToVector requires T to be move- or copy-constructible"); + "Stream::ToVector requires T to be move- or copy-constructible"); // For strictly copy-only T, collecting directly into a vector can repeatedly copy // previously collected elements during vector growth. Stage values in a deque, From 0d3756782038fb3e661616dc5e0f382d0db86fe9 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Thu, 10 Sep 2026 22:40:51 +0800 Subject: [PATCH 16/21] refactor: alias manifest entry streams Co-authored-by: Codex --- src/iceberg/manifest/manifest_group.cc | 4 ++-- src/iceberg/manifest/manifest_reader.cc | 13 ++++++------- src/iceberg/manifest/manifest_reader.h | 11 +++++++---- src/iceberg/manifest/manifest_reader_internal.h | 6 +++--- 4 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/iceberg/manifest/manifest_group.cc b/src/iceberg/manifest/manifest_group.cc index 83fb349c6..bc9f5f39a 100644 --- a/src/iceberg/manifest/manifest_group.cc +++ b/src/iceberg/manifest/manifest_group.cc @@ -221,7 +221,7 @@ class ManifestGroup::FilePlanningStream final drop_stats_(drop_stats) {} using TaggedEntry = std::pair; - using TaggedStream = std::pair>>; + using TaggedStream = std::pair; Result> NextEntry() { if (!group_->executor_.has_value()) { @@ -422,7 +422,7 @@ class ManifestGroup::FilePlanningStream final std::vector columns_; std::unordered_map> manifest_evaluators_; std::unordered_map> residual_evaluators_; - std::unique_ptr> entry_stream_; + ManifestEntryStream entry_stream_; std::vector batch_streams_; size_t next_manifest_ = 0; size_t next_batch_stream_ = 0; diff --git a/src/iceberg/manifest/manifest_reader.cc b/src/iceberg/manifest/manifest_reader.cc index 66b18d35b..0849e299c 100644 --- a/src/iceberg/manifest/manifest_reader.cc +++ b/src/iceberg/manifest/manifest_reader.cc @@ -815,7 +815,7 @@ class ManifestEntryStreamImpl final : public Stream { } // namespace -Result>> ManifestReader::EntriesStream() { +Result ManifestReader::EntriesStream() { if (auto* streaming_reader = dynamic_cast(this)) { return streaming_reader->EntriesStream(); } @@ -823,7 +823,7 @@ Result>> ManifestReader::EntriesStream() { return std::make_unique>(std::move(entries)); } -Result>> ManifestReader::LiveEntriesStream() { +Result ManifestReader::LiveEntriesStream() { if (auto* streaming_reader = dynamic_cast(this)) { return streaming_reader->LiveEntriesStream(); } @@ -1011,16 +1011,15 @@ Result> ManifestReaderImpl::LiveEntries() { return entries->ToVector(); } -Result>> ManifestReaderImpl::EntriesStream() { +Result ManifestReaderImpl::EntriesStream() { return MakeEntriesStream(/*only_live=*/false); } -Result>> ManifestReaderImpl::LiveEntriesStream() { +Result ManifestReaderImpl::LiveEntriesStream() { return MakeEntriesStream(/*only_live=*/true); } -Result>> ManifestReaderImpl::MakeEntriesStream( - bool only_live) { +Result ManifestReaderImpl::MakeEntriesStream(bool only_live) { ICEBERG_ASSIGN_OR_RAISE(auto partition_type, spec_->RawPartitionType(*schema_)); auto data_file_schema = DataFile::Type(std::move(partition_type))->ToSchema(); @@ -1060,7 +1059,7 @@ Result>> ManifestReaderImpl::MakeEntriesSt } bool drop_stats = drop_stats_ && ShouldDropStats(columns_); - auto stream = std::unique_ptr>(new ManifestEntryStreamImpl( + auto stream = ManifestEntryStream(new ManifestEntryStreamImpl( std::move(file_reader_), file_schema_, std::move(arrow_schema), inheritable_metadata_, first_row_id_, is_committed_, only_live, std::move(evaluator), std::move(metrics_evaluator), partition_set_, skip_counter_, diff --git a/src/iceberg/manifest/manifest_reader.h b/src/iceberg/manifest/manifest_reader.h index 61b8e41eb..8ad71f176 100644 --- a/src/iceberg/manifest/manifest_reader.h +++ b/src/iceberg/manifest/manifest_reader.h @@ -37,6 +37,9 @@ namespace iceberg { +/// \brief Owning stream of manifest entries. +using ManifestEntryStream = std::unique_ptr>; + /// \brief Read manifest entries from a manifest file. class ICEBERG_EXPORT ManifestReader { public: @@ -53,14 +56,14 @@ class ICEBERG_EXPORT ManifestReader { /// Implementations using SupportsManifestEntryStreaming produce entries lazily. Other /// implementations are adapted from Entries() for compatibility. The returned stream /// is fallible and single-pass. - Result>> EntriesStream(); + Result EntriesStream(); /// \brief Lazily read only live (non-deleted) manifest entries. /// /// Implementations using SupportsManifestEntryStreaming produce entries lazily. Other /// implementations are adapted from LiveEntries() for compatibility. The returned /// stream is fallible and single-pass. - Result>> LiveEntriesStream(); + Result LiveEntriesStream(); /// \brief Select specific columns of data file to read from the manifest entries. /// @@ -157,13 +160,13 @@ class ICEBERG_EXPORT SupportsManifestEntryStreaming { /// /// The returned stream must own all resources required for consumption and must not /// depend on this reader remaining alive. - virtual Result>> EntriesStream() = 0; + virtual Result EntriesStream() = 0; /// \brief Lazily read only live (non-deleted) manifest entries. /// /// The returned stream must own all resources required for consumption and must not /// depend on this reader remaining alive. - virtual Result>> LiveEntriesStream() = 0; + virtual Result LiveEntriesStream() = 0; }; /// \brief Read manifest files from a manifest list file. diff --git a/src/iceberg/manifest/manifest_reader_internal.h b/src/iceberg/manifest/manifest_reader_internal.h index 9f576ff18..3e28aabcb 100644 --- a/src/iceberg/manifest/manifest_reader_internal.h +++ b/src/iceberg/manifest/manifest_reader_internal.h @@ -66,9 +66,9 @@ class ManifestReaderImpl : public ManifestReader, public SupportsManifestEntrySt Result> LiveEntries() override; - Result>> EntriesStream() override; + Result EntriesStream() override; - Result>> LiveEntriesStream() override; + Result LiveEntriesStream() override; ManifestReader& Select(const std::vector& columns) override; @@ -86,7 +86,7 @@ class ManifestReaderImpl : public ManifestReader, public SupportsManifestEntrySt private: /// \brief Create an entry stream with optional live-only filtering. - Result>> MakeEntriesStream(bool only_live); + Result MakeEntriesStream(bool only_live); /// \brief Lazily open the underlying Avro reader with appropriate schema projection. Status OpenReader(std::shared_ptr projection); From 9277e4e6b640af0de3cf84dd7fc3c386e29c5b7d Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Thu, 10 Sep 2026 22:47:50 +0800 Subject: [PATCH 17/21] docs: document stream APIs Co-authored-by: Codex --- src/iceberg/file_scan_task_stream.h | 1 + src/iceberg/manifest/manifest_reader.h | 1 + src/iceberg/manifest/manifest_reader_internal.h | 2 ++ src/iceberg/util/stream.h | 10 ++++++++++ 4 files changed, 14 insertions(+) diff --git a/src/iceberg/file_scan_task_stream.h b/src/iceberg/file_scan_task_stream.h index f88a8a671..4d181b21b 100644 --- a/src/iceberg/file_scan_task_stream.h +++ b/src/iceberg/file_scan_task_stream.h @@ -30,6 +30,7 @@ namespace iceberg { class FileScanTask; +/// \brief Owning stream of file scan tasks. using FileScanTaskStream = std::unique_ptr>>; } // namespace iceberg diff --git a/src/iceberg/manifest/manifest_reader.h b/src/iceberg/manifest/manifest_reader.h index 8ad71f176..c3383a290 100644 --- a/src/iceberg/manifest/manifest_reader.h +++ b/src/iceberg/manifest/manifest_reader.h @@ -154,6 +154,7 @@ class ICEBERG_EXPORT ManifestReader { /// streaming. class ICEBERG_EXPORT SupportsManifestEntryStreaming { public: + /// \brief Destroy this streaming extension. virtual ~SupportsManifestEntryStreaming() = default; /// \brief Lazily read manifest entries. diff --git a/src/iceberg/manifest/manifest_reader_internal.h b/src/iceberg/manifest/manifest_reader_internal.h index 3e28aabcb..6da96f5ae 100644 --- a/src/iceberg/manifest/manifest_reader_internal.h +++ b/src/iceberg/manifest/manifest_reader_internal.h @@ -66,8 +66,10 @@ class ManifestReaderImpl : public ManifestReader, public SupportsManifestEntrySt Result> LiveEntries() override; + /// \brief Lazily read manifest entries. Result EntriesStream() override; + /// \brief Lazily read only live (non-deleted) manifest entries. Result LiveEntriesStream() override; ManifestReader& Select(const std::vector& columns) override; diff --git a/src/iceberg/util/stream.h b/src/iceberg/util/stream.h index d92d7cbde..97ab07be1 100644 --- a/src/iceberg/util/stream.h +++ b/src/iceberg/util/stream.h @@ -44,12 +44,22 @@ namespace iceberg { template class Stream { public: + /// \brief Destroy this stream and release its producer resources. virtual ~Stream() = default; + /// \brief Construct a stream in its initial state. Stream() = default; + + /// \brief Streams cannot be copied. Stream(const Stream&) = delete; + + /// \brief Streams cannot be copy-assigned. Stream& operator=(const Stream&) = delete; + + /// \brief Move a stream and its terminal state. Stream(Stream&&) noexcept = default; + + /// \brief Move-assign a stream and its terminal state. Stream& operator=(Stream&&) noexcept = default; /// \brief Return the next value, or std::nullopt when the stream is exhausted. From 05268399b333d96b140a5f2cea2cfd7c37423393 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Fri, 11 Sep 2026 00:04:51 +0800 Subject: [PATCH 18/21] refactor: separate stream and owning pointer aliases Co-authored-by: Codex --- src/iceberg/file_scan_task_stream.h | 9 ++++++--- src/iceberg/manifest/manifest_group.cc | 13 ++++++------- src/iceberg/manifest/manifest_group.h | 2 +- src/iceberg/manifest/manifest_reader.cc | 14 +++++++------- src/iceberg/manifest/manifest_reader.h | 15 +++++++++------ src/iceberg/manifest/manifest_reader_internal.h | 6 +++--- src/iceberg/table_scan.cc | 8 ++++---- src/iceberg/table_scan.h | 2 +- src/iceberg/test/stream_test.cc | 6 +++++- 9 files changed, 42 insertions(+), 33 deletions(-) diff --git a/src/iceberg/file_scan_task_stream.h b/src/iceberg/file_scan_task_stream.h index 4d181b21b..f696f955a 100644 --- a/src/iceberg/file_scan_task_stream.h +++ b/src/iceberg/file_scan_task_stream.h @@ -20,7 +20,7 @@ #pragma once /// \file iceberg/file_scan_task_stream.h -/// \brief Define the owning stream type for file scan tasks. +/// \brief Define the stream type for file scan tasks. #include @@ -30,7 +30,10 @@ namespace iceberg { class FileScanTask; -/// \brief Owning stream of file scan tasks. -using FileScanTaskStream = std::unique_ptr>>; +/// \brief Stream of file scan tasks. +using FileScanTaskStream = Stream>; + +/// \brief Owning pointer to a file scan task stream. +using FileScanTaskStreamPtr = std::unique_ptr; } // namespace iceberg diff --git a/src/iceberg/manifest/manifest_group.cc b/src/iceberg/manifest/manifest_group.cc index bc9f5f39a..f6e478ad5 100644 --- a/src/iceberg/manifest/manifest_group.cc +++ b/src/iceberg/manifest/manifest_group.cc @@ -131,10 +131,9 @@ ManifestGroup::~ManifestGroup() = default; ManifestGroup::ManifestGroup(ManifestGroup&&) noexcept = default; ManifestGroup& ManifestGroup::operator=(ManifestGroup&&) noexcept = default; -class ManifestGroup::FilePlanningStream final - : public Stream> { +class ManifestGroup::FilePlanningStream final : public FileScanTaskStream { public: - static Result Make(std::unique_ptr group) { + static Result Make(std::unique_ptr group) { ICEBERG_RETURN_UNEXPECTED(group->CheckErrors()); group->delete_index_builder_.WithScanMetrics(group->scan_metrics_); @@ -153,7 +152,7 @@ class ManifestGroup::FilePlanningStream final } const bool drop_stats = stats_projection.drop_stats; - return FileScanTaskStream(new FilePlanningStream( + return FileScanTaskStreamPtr(new FilePlanningStream( std::move(group), std::move(delete_index), std::move(data_file_evaluator), std::move(stats_projection.columns), drop_stats)); } @@ -221,7 +220,7 @@ class ManifestGroup::FilePlanningStream final drop_stats_(drop_stats) {} using TaggedEntry = std::pair; - using TaggedStream = std::pair; + using TaggedStream = std::pair; Result> NextEntry() { if (!group_->executor_.has_value()) { @@ -422,7 +421,7 @@ class ManifestGroup::FilePlanningStream final std::vector columns_; std::unordered_map> manifest_evaluators_; std::unordered_map> residual_evaluators_; - ManifestEntryStream entry_stream_; + ManifestEntryStreamPtr entry_stream_; std::vector batch_streams_; size_t next_manifest_ = 0; size_t next_batch_stream_ = 0; @@ -559,7 +558,7 @@ Result>> ManifestGroup::PlanFiles() { return file_tasks; } -Result ManifestGroup::PlanFilesStream() && { +Result ManifestGroup::PlanFilesStream() && { auto group = std::make_unique(std::move(*this)); return FilePlanningStream::Make(std::move(group)); } diff --git a/src/iceberg/manifest/manifest_group.h b/src/iceberg/manifest/manifest_group.h index 36ce7814a..5b736eb1d 100644 --- a/src/iceberg/manifest/manifest_group.h +++ b/src/iceberg/manifest/manifest_group.h @@ -147,7 +147,7 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector { /// stream because delete files must be indexed before data-file planning can begin. /// Creating the stream consumes this group's configuration, so this method may only /// be called on an rvalue. - Result PlanFilesStream() &&; + Result PlanFilesStream() &&; /// \brief Get all matching manifest entries. Result> Entries(); diff --git a/src/iceberg/manifest/manifest_reader.cc b/src/iceberg/manifest/manifest_reader.cc index 0849e299c..d0894631d 100644 --- a/src/iceberg/manifest/manifest_reader.cc +++ b/src/iceberg/manifest/manifest_reader.cc @@ -707,7 +707,7 @@ class VectorStream final : public Stream { size_t next_ = 0; }; -class ManifestEntryStreamImpl final : public Stream { +class ManifestEntryStreamImpl final : public ManifestEntryStream { public: ManifestEntryStreamImpl(std::unique_ptr reader, std::shared_ptr file_schema, ArrowSchema arrow_schema, @@ -815,7 +815,7 @@ class ManifestEntryStreamImpl final : public Stream { } // namespace -Result ManifestReader::EntriesStream() { +Result ManifestReader::EntriesStream() { if (auto* streaming_reader = dynamic_cast(this)) { return streaming_reader->EntriesStream(); } @@ -823,7 +823,7 @@ Result ManifestReader::EntriesStream() { return std::make_unique>(std::move(entries)); } -Result ManifestReader::LiveEntriesStream() { +Result ManifestReader::LiveEntriesStream() { if (auto* streaming_reader = dynamic_cast(this)) { return streaming_reader->LiveEntriesStream(); } @@ -1011,15 +1011,15 @@ Result> ManifestReaderImpl::LiveEntries() { return entries->ToVector(); } -Result ManifestReaderImpl::EntriesStream() { +Result ManifestReaderImpl::EntriesStream() { return MakeEntriesStream(/*only_live=*/false); } -Result ManifestReaderImpl::LiveEntriesStream() { +Result ManifestReaderImpl::LiveEntriesStream() { return MakeEntriesStream(/*only_live=*/true); } -Result ManifestReaderImpl::MakeEntriesStream(bool only_live) { +Result ManifestReaderImpl::MakeEntriesStream(bool only_live) { ICEBERG_ASSIGN_OR_RAISE(auto partition_type, spec_->RawPartitionType(*schema_)); auto data_file_schema = DataFile::Type(std::move(partition_type))->ToSchema(); @@ -1059,7 +1059,7 @@ Result ManifestReaderImpl::MakeEntriesStream(bool only_live } bool drop_stats = drop_stats_ && ShouldDropStats(columns_); - auto stream = ManifestEntryStream(new ManifestEntryStreamImpl( + auto stream = ManifestEntryStreamPtr(new ManifestEntryStreamImpl( std::move(file_reader_), file_schema_, std::move(arrow_schema), inheritable_metadata_, first_row_id_, is_committed_, only_live, std::move(evaluator), std::move(metrics_evaluator), partition_set_, skip_counter_, diff --git a/src/iceberg/manifest/manifest_reader.h b/src/iceberg/manifest/manifest_reader.h index c3383a290..42fe1e164 100644 --- a/src/iceberg/manifest/manifest_reader.h +++ b/src/iceberg/manifest/manifest_reader.h @@ -37,8 +37,11 @@ namespace iceberg { -/// \brief Owning stream of manifest entries. -using ManifestEntryStream = std::unique_ptr>; +/// \brief Stream of manifest entries. +using ManifestEntryStream = Stream; + +/// \brief Owning pointer to a manifest entry stream. +using ManifestEntryStreamPtr = std::unique_ptr; /// \brief Read manifest entries from a manifest file. class ICEBERG_EXPORT ManifestReader { @@ -56,14 +59,14 @@ class ICEBERG_EXPORT ManifestReader { /// Implementations using SupportsManifestEntryStreaming produce entries lazily. Other /// implementations are adapted from Entries() for compatibility. The returned stream /// is fallible and single-pass. - Result EntriesStream(); + Result EntriesStream(); /// \brief Lazily read only live (non-deleted) manifest entries. /// /// Implementations using SupportsManifestEntryStreaming produce entries lazily. Other /// implementations are adapted from LiveEntries() for compatibility. The returned /// stream is fallible and single-pass. - Result LiveEntriesStream(); + Result LiveEntriesStream(); /// \brief Select specific columns of data file to read from the manifest entries. /// @@ -161,13 +164,13 @@ class ICEBERG_EXPORT SupportsManifestEntryStreaming { /// /// The returned stream must own all resources required for consumption and must not /// depend on this reader remaining alive. - virtual Result EntriesStream() = 0; + virtual Result EntriesStream() = 0; /// \brief Lazily read only live (non-deleted) manifest entries. /// /// The returned stream must own all resources required for consumption and must not /// depend on this reader remaining alive. - virtual Result LiveEntriesStream() = 0; + virtual Result LiveEntriesStream() = 0; }; /// \brief Read manifest files from a manifest list file. diff --git a/src/iceberg/manifest/manifest_reader_internal.h b/src/iceberg/manifest/manifest_reader_internal.h index 6da96f5ae..5aa7c49b1 100644 --- a/src/iceberg/manifest/manifest_reader_internal.h +++ b/src/iceberg/manifest/manifest_reader_internal.h @@ -67,10 +67,10 @@ class ManifestReaderImpl : public ManifestReader, public SupportsManifestEntrySt Result> LiveEntries() override; /// \brief Lazily read manifest entries. - Result EntriesStream() override; + Result EntriesStream() override; /// \brief Lazily read only live (non-deleted) manifest entries. - Result LiveEntriesStream() override; + Result LiveEntriesStream() override; ManifestReader& Select(const std::vector& columns) override; @@ -88,7 +88,7 @@ class ManifestReaderImpl : public ManifestReader, public SupportsManifestEntrySt private: /// \brief Create an entry stream with optional live-only filtering. - Result MakeEntriesStream(bool only_live); + Result MakeEntriesStream(bool only_live); /// \brief Lazily open the underlying Avro reader with appropriate schema projection. Status OpenReader(std::shared_ptr projection); diff --git a/src/iceberg/table_scan.cc b/src/iceberg/table_scan.cc index cefb000bd..d13de73e9 100644 --- a/src/iceberg/table_scan.cc +++ b/src/iceberg/table_scan.cc @@ -108,9 +108,9 @@ Result MakeScanReport(const DataTableScan& scan, const Snapshot& sna }; } -class ReportingFileTaskStream final : public Stream> { +class ReportingFileTaskStream final : public FileScanTaskStream { public: - ReportingFileTaskStream(FileScanTaskStream stream, + ReportingFileTaskStream(FileScanTaskStreamPtr stream, std::shared_ptr scan_metrics, std::chrono::nanoseconds planning_duration, std::shared_ptr reporter, ScanReport report) @@ -147,7 +147,7 @@ class ReportingFileTaskStream final : public StreamReport(report_); } - FileScanTaskStream stream_; + FileScanTaskStreamPtr stream_; std::shared_ptr scan_metrics_; std::chrono::nanoseconds planning_duration_; std::shared_ptr reporter_; @@ -721,7 +721,7 @@ Result>> DataTableScan::PlanFiles() co return tasks; } -Result DataTableScan::PlanFilesStream() const { +Result DataTableScan::PlanFilesStream() const { ICEBERG_ASSIGN_OR_RAISE(auto snapshot, this->snapshot()); if (!snapshot) { return std::make_unique>>(); diff --git a/src/iceberg/table_scan.h b/src/iceberg/table_scan.h index b68bae05f..467816e6c 100644 --- a/src/iceberg/table_scan.h +++ b/src/iceberg/table_scan.h @@ -469,7 +469,7 @@ class ICEBERG_EXPORT DataTableScan : public TableScan { /// Unlike PlanFiles(), this method does not materialize all manifest entries and scan /// tasks. The returned fallible, single-pass stream owns its planning resources and /// can outlive this scan. - Result PlanFilesStream() const; + Result PlanFilesStream() const; private: Status ReportScan(const Snapshot& snapshot, const ScanMetrics& scan_metrics) const; diff --git a/src/iceberg/test/stream_test.cc b/src/iceberg/test/stream_test.cc index ebb126e60..dd63244b0 100644 --- a/src/iceberg/test/stream_test.cc +++ b/src/iceberg/test/stream_test.cc @@ -104,7 +104,11 @@ static_assert(std::is_move_constructible_v); static_assert(std::is_move_assignable_v); TEST(StreamTest, FileScanTaskStreamSupportsIncompleteFileScanTask) { - FileScanTaskStream stream; + static_assert( + std::is_same_v>>); + static_assert( + std::is_same_v>); + FileScanTaskStreamPtr stream; EXPECT_EQ(stream, nullptr); } From 799c47419389921bdc63b652f76aaa2eb685c7b0 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Fri, 11 Sep 2026 00:47:20 +0800 Subject: [PATCH 19/21] refactor: consolidate file scan task stream aliases Co-authored-by: Codex --- src/iceberg/file_scan_task_stream.h | 39 --------------------------- src/iceberg/manifest/manifest_group.h | 2 +- src/iceberg/meson.build | 1 - src/iceberg/table_scan.h | 8 +++++- src/iceberg/test/stream_test.cc | 12 ++++----- 5 files changed, 13 insertions(+), 49 deletions(-) delete mode 100644 src/iceberg/file_scan_task_stream.h diff --git a/src/iceberg/file_scan_task_stream.h b/src/iceberg/file_scan_task_stream.h deleted file mode 100644 index f696f955a..000000000 --- a/src/iceberg/file_scan_task_stream.h +++ /dev/null @@ -1,39 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -#pragma once - -/// \file iceberg/file_scan_task_stream.h -/// \brief Define the stream type for file scan tasks. - -#include - -#include "iceberg/util/stream.h" - -namespace iceberg { - -class FileScanTask; - -/// \brief Stream of file scan tasks. -using FileScanTaskStream = Stream>; - -/// \brief Owning pointer to a file scan task stream. -using FileScanTaskStreamPtr = std::unique_ptr; - -} // namespace iceberg diff --git a/src/iceberg/manifest/manifest_group.h b/src/iceberg/manifest/manifest_group.h index 5b736eb1d..ef1468e14 100644 --- a/src/iceberg/manifest/manifest_group.h +++ b/src/iceberg/manifest/manifest_group.h @@ -30,11 +30,11 @@ #include #include "iceberg/delete_file_index.h" -#include "iceberg/file_scan_task_stream.h" #include "iceberg/iceberg_export.h" #include "iceberg/manifest/manifest_entry.h" #include "iceberg/manifest/manifest_list.h" #include "iceberg/result.h" +#include "iceberg/table_scan.h" #include "iceberg/type_fwd.h" #include "iceberg/util/error_collector.h" #include "iceberg/util/executor.h" diff --git a/src/iceberg/meson.build b/src/iceberg/meson.build index 2f236a106..989f4ae03 100644 --- a/src/iceberg/meson.build +++ b/src/iceberg/meson.build @@ -326,7 +326,6 @@ install_headers( 'file_io.h', 'file_io_registry.h', 'file_reader.h', - 'file_scan_task_stream.h', 'file_writer.h', 'geospatial.h', 'iceberg_data_export.h', diff --git a/src/iceberg/table_scan.h b/src/iceberg/table_scan.h index 467816e6c..7d2a18bdd 100644 --- a/src/iceberg/table_scan.h +++ b/src/iceberg/table_scan.h @@ -30,13 +30,13 @@ #include #include -#include "iceberg/file_scan_task_stream.h" #include "iceberg/iceberg_export.h" #include "iceberg/result.h" #include "iceberg/table_metadata.h" #include "iceberg/type_fwd.h" #include "iceberg/util/error_collector.h" #include "iceberg/util/executor.h" +#include "iceberg/util/stream.h" namespace iceberg { @@ -97,6 +97,12 @@ class ICEBERG_EXPORT FileScanTask : public ScanTask { std::shared_ptr residual_filter_; }; +/// \brief Stream of file scan tasks. +using FileScanTaskStream = Stream>; + +/// \brief Owning pointer to a file scan task stream. +using FileScanTaskStreamPtr = std::unique_ptr; + enum class ChangelogOperation : uint8_t { kInsert, kDelete, diff --git a/src/iceberg/test/stream_test.cc b/src/iceberg/test/stream_test.cc index dd63244b0..91b352167 100644 --- a/src/iceberg/test/stream_test.cc +++ b/src/iceberg/test/stream_test.cc @@ -17,6 +17,8 @@ * under the License. */ +#include "iceberg/util/stream.h" + #include #include #include @@ -24,7 +26,6 @@ #include -#include "iceberg/file_scan_task_stream.h" #include "iceberg/test/matchers.h" namespace iceberg { @@ -103,12 +104,9 @@ static_assert(std::is_move_assignable_v); static_assert(std::is_move_constructible_v); static_assert(std::is_move_assignable_v); -TEST(StreamTest, FileScanTaskStreamSupportsIncompleteFileScanTask) { - static_assert( - std::is_same_v>>); - static_assert( - std::is_same_v>); - FileScanTaskStreamPtr stream; +TEST(StreamTest, SupportsIncompleteSharedPointerValue) { + class IncompleteType; + std::unique_ptr>> stream; EXPECT_EQ(stream, nullptr); } From 8317b0e70db9642cd8454aa5f5925e5616601da3 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Fri, 11 Sep 2026 11:06:54 +0900 Subject: [PATCH 20/21] fix: make manifest entry streams self-contained --- src/iceberg/manifest/manifest_reader.h | 1 + src/iceberg/test/meson.build | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/iceberg/manifest/manifest_reader.h b/src/iceberg/manifest/manifest_reader.h index 42fe1e164..a941206e4 100644 --- a/src/iceberg/manifest/manifest_reader.h +++ b/src/iceberg/manifest/manifest_reader.h @@ -30,6 +30,7 @@ #include #include "iceberg/iceberg_export.h" +#include "iceberg/manifest/manifest_entry.h" #include "iceberg/metrics/counter.h" #include "iceberg/result.h" #include "iceberg/type_fwd.h" diff --git a/src/iceberg/test/meson.build b/src/iceberg/test/meson.build index 4f00bdff5..c1ca0c327 100644 --- a/src/iceberg/test/meson.build +++ b/src/iceberg/test/meson.build @@ -110,7 +110,6 @@ iceberg_tests = { 'executor_util_test.cc', 'file_io_test.cc', 'formatter_test.cc', - 'stream_test.cc', 'lazy_test.cc', 'location_util_test.cc', 'math_util_internal_test.cc', @@ -119,6 +118,7 @@ iceberg_tests = { 'resolving_file_io_test.cc', 'retry_util_test.cc', 'roaring_position_bitmap_test.cc', + 'stream_test.cc', 'string_util_test.cc', 'struct_like_set_test.cc', 'task_group_test.cc', From e0e49c690aa1983fd23a5d4ec9cdea5238be56e5 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Fri, 11 Sep 2026 11:37:39 +0900 Subject: [PATCH 21/21] docs: clarify planning executor lifetime --- src/iceberg/manifest/manifest_group.h | 6 ++++++ src/iceberg/table_scan.h | 7 ++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/iceberg/manifest/manifest_group.h b/src/iceberg/manifest/manifest_group.h index ef1468e14..d2b17ef04 100644 --- a/src/iceberg/manifest/manifest_group.h +++ b/src/iceberg/manifest/manifest_group.h @@ -127,6 +127,9 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector { /// \brief Configure an optional executor for manifest planning. /// + /// The executor is borrowed and must remain alive throughout planning and until any + /// stream returned by PlanFilesStream() is destroyed. + /// /// \param executor Executor to use, or std::nullopt to plan manifests serially. /// \return Reference to this for method chaining. ManifestGroup& PlanWith(OptionalExecutor executor); @@ -140,6 +143,9 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector { /// \brief Lazily plan scan tasks for matching data files. /// /// The returned stream owns the planning state and may outlive this ManifestGroup. + /// An executor configured through PlanWith() is borrowed and must remain alive until + /// the stream is destroyed, as later Next() calls may submit work to it. + /// /// It reads one bounded manifest batch at a time instead of materializing all manifest /// entries and scan tasks. When PlanWith() configures an executor, entry streams for /// manifests in each batch are opened in parallel, while entries are consumed one diff --git a/src/iceberg/table_scan.h b/src/iceberg/table_scan.h index 7d2a18bdd..4d81c5787 100644 --- a/src/iceberg/table_scan.h +++ b/src/iceberg/table_scan.h @@ -311,6 +311,9 @@ class ICEBERG_TEMPLATE_CLASS_EXPORT TableScanBuilder : public ErrorCollector { /// \brief Configure an executor for manifest planning. /// + /// The executor is borrowed and must remain alive throughout planning by scans built + /// from this builder and until any stream returned by PlanFilesStream() is destroyed. + /// /// \param executor Executor to use while planning manifests. /// \return Reference to this for method chaining. TableScanBuilder& PlanWith(Executor& executor); @@ -474,7 +477,9 @@ class ICEBERG_EXPORT DataTableScan : public TableScan { /// /// Unlike PlanFiles(), this method does not materialize all manifest entries and scan /// tasks. The returned fallible, single-pass stream owns its planning resources and - /// can outlive this scan. + /// can outlive this scan. An executor configured through PlanWith() is borrowed and + /// must remain alive until the stream is destroyed, as later Next() calls may submit + /// work to it. Result PlanFilesStream() const; private: