Skip to content

Commit 7b64150

Browse files
manuzhangcodex
andcommitted
fix: align manifest planning stats semantics
Co-authored-by: Codex <codex@openai.com>
1 parent b9e5583 commit 7b64150

3 files changed

Lines changed: 23 additions & 8 deletions

File tree

src/iceberg/manifest/manifest_group.cc

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -142,10 +142,8 @@ class ManifestGroup::FilePlanningIterator final
142142
group->delete_index_builder_.WithScanMetrics(group->scan_metrics_);
143143
ICEBERG_ASSIGN_OR_RAISE(auto delete_index, group->delete_index_builder_.Build());
144144

145-
const bool drop_stats = ManifestReader::ShouldDropStats(group->columns_);
146-
if (delete_index->has_equality_deletes()) {
147-
group->columns_ = ManifestReader::WithStatsColumns(group->columns_);
148-
}
145+
const bool drop_stats =
146+
group->PrepareStatsProjection(delete_index->has_equality_deletes());
149147

150148
std::unique_ptr<Evaluator> data_file_evaluator;
151149
if (group->file_filter_ &&
@@ -426,6 +424,10 @@ class ManifestGroup::FilePlanningIterator final
426424
int32_t current_spec_id_ = 0;
427425
bool drop_stats_;
428426

427+
// Limit the number of manifest readers and iterators retained by executor-backed
428+
// planning. The executor still controls actual task concurrency, while this fixed
429+
// cap prevents resource use from scaling with the total manifest count. Entries
430+
// within each manifest remain streamed, so this does not cap manifest size.
429431
static constexpr size_t kManifestReadBatchSize = 32;
430432
};
431433

@@ -583,10 +585,7 @@ Result<std::vector<std::shared_ptr<ScanTask>>> ManifestGroup::Plan(
583585
delete_index_builder_.WithScanMetrics(scan_metrics_);
584586
ICEBERG_ASSIGN_OR_RAISE(auto delete_index, delete_index_builder_.Build());
585587

586-
bool drop_stats = ManifestReader::ShouldDropStats(columns_);
587-
if (delete_index->has_equality_deletes()) {
588-
columns_ = ManifestReader::WithStatsColumns(columns_);
589-
}
588+
const bool drop_stats = PrepareStatsProjection(delete_index->has_equality_deletes());
590589

591590
std::unordered_map<int32_t, std::unique_ptr<TaskContext>> task_context_cache;
592591
auto get_task_context = [&](int32_t spec_id) -> Result<TaskContext*> {
@@ -680,6 +679,18 @@ Result<std::unique_ptr<ManifestReader>> ManifestGroup::MakeReader(
680679
return reader;
681680
}
682681

682+
bool ManifestGroup::PrepareStatsProjection(bool has_equality_deletes) {
683+
// The caller's projection records whether stats were requested. Equality-delete
684+
// matching may add stats temporarily, but they should still be dropped from the
685+
// result when the original projection did not request them. Keeping this decision
686+
// here ensures eager and iterator planning use identical semantics.
687+
const bool drop_stats = ManifestReader::ShouldDropStats(columns_);
688+
if (has_equality_deletes) {
689+
columns_ = ManifestReader::WithStatsColumns(columns_);
690+
}
691+
return drop_stats;
692+
}
693+
683694
Result<std::unordered_map<int32_t, std::vector<ManifestEntry>>>
684695
ManifestGroup::ReadEntries() {
685696
// TODO(zehua): Replace with a thread-safe LRU cache.

src/iceberg/manifest/manifest_group.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,8 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector {
173173

174174
Result<std::unique_ptr<ManifestReader>> MakeReader(const ManifestFile& manifest);
175175

176+
bool PrepareStatsProjection(bool has_equality_deletes);
177+
176178
std::shared_ptr<FileIO> io_;
177179
std::shared_ptr<Schema> schema_;
178180
std::unordered_map<int32_t, std::shared_ptr<PartitionSpec>> specs_by_id_;

src/iceberg/test/manifest_group_test.cc

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,8 @@ TEST_P(ManifestGroupTest, PlanFilesIteratorPreservesSelectAllWithEqualityDeletes
329329
ASSERT_TRUE(task.has_value());
330330
EXPECT_EQ(task.value()->data_file()->file_path, "/path/to/data.parquet");
331331
EXPECT_EQ(task.value()->data_file()->record_count, 100);
332+
EXPECT_TRUE(task.value()->data_file()->lower_bounds.contains(1));
333+
EXPECT_TRUE(task.value()->data_file()->upper_bounds.contains(1));
332334
ASSERT_EQ(task.value()->delete_files().size(), 1);
333335
EXPECT_EQ(task.value()->delete_files().front()->file_path,
334336
"/path/to/equality-delete.parquet");

0 commit comments

Comments
 (0)