You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "HuaHuaY (via GitHub)" <gi...@apache.org> on 2026/09/21 10:03:05 UTC
[PR] refactor: simplify src/iceberg/manifest/manifest_group.cc [iceberg-cpp]
HuaHuaY opened a new pull request, #952:
URL: https://github.com/apache/iceberg-cpp/pull/952
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: simplify src/iceberg/manifest/manifest_group.cc [iceberg-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #952:
URL: https://github.com/apache/iceberg-cpp/pull/952#discussion_r4061090098
##########
src/iceberg/manifest/manifest_group.cc:
##########
@@ -296,67 +376,23 @@ class ManifestGroup::FilePlanningStream final : public FileScanTaskStream {
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_));
+ ICEBERG_ASSIGN_OR_RAISE(auto evaluator, context_.MakeResidualEvaluator(spec_id));
auto* result = evaluator.get();
residual_evaluators_.emplace(spec_id, std::move(evaluator));
return result;
}
- Result<bool> 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));
- 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;
- }
-
- if (group_->scan_metrics_) {
- group_->scan_metrics_->scanned_data_manifests->Increment(1);
- }
- return true;
- }
-
- Result<bool> OpenNextManifest() {
- while (next_manifest_ < group_->data_manifests_.size()) {
- const auto& manifest = group_->data_manifests_[next_manifest_++];
- ICEBERG_ASSIGN_OR_RAISE(bool should_read, ShouldReadManifest(manifest));
- if (!should_read) {
- continue;
- }
-
- ICEBERG_ASSIGN_OR_RAISE(auto reader, group_->MakeReader(manifest, columns_));
- ICEBERG_ASSIGN_OR_RAISE(entry_stream_, group_->ignore_deleted_
- ? reader->LiveEntriesStream()
- : reader->EntriesStream());
- current_spec_id_ = manifest.partition_spec_id;
- return true;
- }
- return false;
- }
-
Result<bool> LoadNextManifestBatch() {
+ const size_t batch_size = group_->executor_.has_value() ? kManifestReadBatchSize : 1;
std::vector<const ManifestFile*> manifests;
- manifests.reserve(kManifestReadBatchSize);
+ manifests.reserve(batch_size);
while (next_manifest_ < group_->data_manifests_.size() &&
- manifests.size() < kManifestReadBatchSize) {
+ manifests.size() < batch_size) {
Review Comment:
`NextEntry()` treats a `false` return from `LoadNextManifestBatch()` as end-of-stream, but this loader can return `false` when the current batch contains only filtered manifests while `next_manifest_` still has input. With `batch_size == 1`, any skipped manifest before a matching one now truncates serial planning; an all-skipped executor batch has the same problem. Continue loading batches until one is non-empty or all manifests have been examined, and add a regression case with a skipped manifest before a match.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: simplify src/iceberg/manifest/manifest_group.cc [iceberg-cpp]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #952:
URL: https://github.com/apache/iceberg-cpp/pull/952#issuecomment-5817236090
@HuaHuaY Could you please add a summary of the refactor in the PR description?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: simplify src/iceberg/manifest/manifest_group.cc [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #952:
URL: https://github.com/apache/iceberg-cpp/pull/952#issuecomment-6092980143
Nice refactoring!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: simplify src/iceberg/manifest/manifest_group.cc [iceberg-cpp]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #952:
URL: https://github.com/apache/iceberg-cpp/pull/952#discussion_r4061148623
##########
src/iceberg/manifest/manifest_group.cc:
##########
@@ -296,67 +376,23 @@ class ManifestGroup::FilePlanningStream final : public FileScanTaskStream {
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_));
+ ICEBERG_ASSIGN_OR_RAISE(auto evaluator, context_.MakeResidualEvaluator(spec_id));
auto* result = evaluator.get();
residual_evaluators_.emplace(spec_id, std::move(evaluator));
return result;
}
- Result<bool> 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));
- 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;
- }
-
- if (group_->scan_metrics_) {
- group_->scan_metrics_->scanned_data_manifests->Increment(1);
- }
- return true;
- }
-
- Result<bool> OpenNextManifest() {
- while (next_manifest_ < group_->data_manifests_.size()) {
- const auto& manifest = group_->data_manifests_[next_manifest_++];
- ICEBERG_ASSIGN_OR_RAISE(bool should_read, ShouldReadManifest(manifest));
- if (!should_read) {
- continue;
- }
-
- ICEBERG_ASSIGN_OR_RAISE(auto reader, group_->MakeReader(manifest, columns_));
- ICEBERG_ASSIGN_OR_RAISE(entry_stream_, group_->ignore_deleted_
- ? reader->LiveEntriesStream()
- : reader->EntriesStream());
- current_spec_id_ = manifest.partition_spec_id;
- return true;
- }
- return false;
- }
-
Result<bool> LoadNextManifestBatch() {
+ const size_t batch_size = group_->executor_.has_value() ? kManifestReadBatchSize : 1;
std::vector<const ManifestFile*> manifests;
- manifests.reserve(kManifestReadBatchSize);
+ manifests.reserve(batch_size);
while (next_manifest_ < group_->data_manifests_.size() &&
- manifests.size() < kManifestReadBatchSize) {
+ manifests.size() < batch_size) {
Review Comment:
This is a false positive. I'll add one more comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: simplify src/iceberg/manifest/manifest_group.cc [iceberg-cpp]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #952:
URL: https://github.com/apache/iceberg-cpp/pull/952#discussion_r4202716201
##########
src/iceberg/manifest/manifest_group.cc:
##########
@@ -211,40 +329,16 @@ class ManifestGroup::FilePlanningStream final : public FileScanTaskStream {
private:
FilePlanningStream(std::unique_ptr<ManifestGroup> group,
std::unique_ptr<DeleteFileIndex> delete_index,
- std::unique_ptr<Evaluator> data_file_evaluator,
- std::vector<std::string> columns, bool drop_stats)
+ PlanningContext context, 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)),
+ context_(std::move(context)),
drop_stats_(drop_stats) {}
using TaggedEntry = std::pair<int32_t, ManifestEntry>;
using TaggedStream = std::pair<int32_t, ManifestEntryStreamPtr>;
- // FIXME: Perhaps refactor this concurrent/sequential stream state machine into a
Review Comment:
I restored the comments. Since this PR unifies the serial and parallel code, I made some minor adjustments to the comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: simplify src/iceberg/manifest/manifest_group.cc [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #952:
URL: https://github.com/apache/iceberg-cpp/pull/952
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: simplify src/iceberg/manifest/manifest_group.cc [iceberg-cpp]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #952:
URL: https://github.com/apache/iceberg-cpp/pull/952#discussion_r4095379266
##########
src/iceberg/manifest/manifest_group.cc:
##########
@@ -211,40 +329,16 @@ class ManifestGroup::FilePlanningStream final : public FileScanTaskStream {
private:
FilePlanningStream(std::unique_ptr<ManifestGroup> group,
std::unique_ptr<DeleteFileIndex> delete_index,
- std::unique_ptr<Evaluator> data_file_evaluator,
- std::vector<std::string> columns, bool drop_stats)
+ PlanningContext context, 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)),
+ context_(std::move(context)),
drop_stats_(drop_stats) {}
using TaggedEntry = std::pair<int32_t, ManifestEntry>;
using TaggedStream = std::pair<int32_t, ManifestEntryStreamPtr>;
- // FIXME: Perhaps refactor this concurrent/sequential stream state machine into a
Review Comment:
Is this fixed now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: simplify src/iceberg/manifest/manifest_group.cc [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #952:
URL: https://github.com/apache/iceberg-cpp/pull/952#issuecomment-5811817821
Could you take a look at this? @manuzhang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: simplify src/iceberg/manifest/manifest_group.cc [iceberg-cpp]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #952:
URL: https://github.com/apache/iceberg-cpp/pull/952#discussion_r4095137999
##########
src/iceberg/manifest/manifest_group.cc:
##########
@@ -131,6 +131,148 @@ ManifestGroup::~ManifestGroup() = default;
ManifestGroup::ManifestGroup(ManifestGroup&&) noexcept = default;
ManifestGroup& ManifestGroup::operator=(ManifestGroup&&) noexcept = default;
+class ManifestGroup::PlanningContext {
+ public:
+ // `group` is borrowed; its planning configuration must remain unchanged for the
+ // lifetime of this context.
+ static Result<PlanningContext> Make(const ManifestGroup& group,
+ std::vector<std::string> columns) {
+ std::unique_ptr<Evaluator> file_evaluator;
+ if (!group.file_filter_ || group.file_filter_->op() == Expression::Operation::kTrue) {
+ return PlanningContext(group, std::move(file_evaluator), std::move(columns));
+ }
+
+ auto data_file_schema = DataFileFilterSchema();
+ ICEBERG_ASSIGN_OR_RAISE(
+ file_evaluator,
+ Evaluator::Make(*data_file_schema, group.file_filter_, group.case_sensitive_));
+ if (std::ranges::contains(columns, Schema::kAllColumns)) {
+ return PlanningContext(group, std::move(file_evaluator), std::move(columns));
+ }
+
+ ICEBERG_ASSIGN_OR_RAISE(
+ auto bound_file_filter,
+ Binder::Bind(*data_file_schema, group.file_filter_, group.case_sensitive_));
+ ICEBERG_ASSIGN_OR_RAISE(auto referenced_field_ids,
+ ReferenceVisitor::GetReferencedFieldIds(bound_file_filter));
+
+ std::unordered_set<std::string> selected_columns(columns.cbegin(), columns.cend());
+ for (const auto field_id : referenced_field_ids) {
Review Comment:
nit: I'm a bit uncomfortable with the follow two `continue`s in one loop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org