Copilot commented on code in PR #873:
URL: https://github.com/apache/iceberg-cpp/pull/873#discussion_r3795548951
##########
src/iceberg/manifest/manifest_group.cc:
##########
@@ -132,6 +132,223 @@ ManifestGroup::~ManifestGroup() = default;
ManifestGroup::ManifestGroup(ManifestGroup&&) noexcept = default;
ManifestGroup& ManifestGroup::operator=(ManifestGroup&&) noexcept = default;
+class ManifestGroup::FilePlanningIterator final
+ : public Iterator<std::shared_ptr<FileScanTask>> {
+ public:
+ static Result<std::unique_ptr<Iterator<std::shared_ptr<FileScanTask>>>> Make(
+ std::unique_ptr<ManifestGroup> 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_);
+ }
Review Comment:
`drop_stats` is computed before potentially mutating `group->columns_` for
equality deletes. If `WithStatsColumns()` changes the outcome of
`ShouldDropStats()`, the iterator may incorrectly drop stats even though
equality-delete planning requires them. Compute `drop_stats` after the
equality-delete column adjustment (or recompute it after mutation) so the
drop/keep decision matches the final projected columns.
##########
src/iceberg/manifest/manifest_reader.cc:
##########
@@ -894,74 +1040,29 @@ Result<std::vector<ManifestEntry>>
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<ManifestEntry> 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> evaluator;
+ std::unique_ptr<InclusiveMetricsEvaluator> metrics_evaluator;
if (HasPartitionFilter() || HasRowFilter()) {
- ICEBERG_ASSIGN_OR_RAISE(evaluator, GetEvaluator());
+ ICEBERG_RETURN_UNEXPECTED(GetEvaluator());
+ evaluator = std::move(evaluator_);
}
if (HasRowFilter()) {
- ICEBERG_ASSIGN_OR_RAISE(metrics_evaluator, GetMetricsEvaluator());
+ ICEBERG_RETURN_UNEXPECTED(GetMetricsEvaluator());
+ metrics_evaluator = std::move(metrics_evaluator_);
}
Review Comment:
The results of `GetEvaluator()` / `GetMetricsEvaluator()` are discarded, and
the code instead moves `evaluator_` / `metrics_evaluator_` members. This is
fragile and can be incorrect if those functions return the evaluator rather
than populating members as a side effect. Prefer directly unwrapping into the
local `std::unique_ptr` (e.g., via the existing `ICEBERG_ASSIGN_OR_RAISE`) or
refactor the helpers to return the evaluator explicitly and use that return
value here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
