feat(parquet): add statistics-based row group filtering by YinZheng-Sun · Pull Request #956 · apache/iceberg-cpp

YinZheng-Sun · 2026-09-22T03:25:18Z

What

Adds ParquetMetricsRowGroupFilter to evaluate Iceberg predicates against Parquet footer statistics before reading row groups.

Adds tests covering split intersection, non-contiguous row groups, physical row positions, unprojected filter columns, schema evolution, missing statistics, predicate binding, and delete handling.

Why

The Parquet reader previously selected row groups only by file split. Filter predicates could not eliminate row groups whose footer statistics ruled out matching rows.

Behavior change

Enables statistics-based pruning by default through read.parquet.row-group-filter.enabled.
Retained row groups still require residual predicate evaluation; this does not implement exact row filtering.
Batches stop at row-group boundaries, including when pruning is disabled.

Testing

Built and ran parquet_test, data_test, avro_test, and expression_test locally during implementation. Rebuilt and reran parquet_test and data_test after the binding and field-mapping refactors; both passed.

The IN regression covers 201 input literals that deduplicate to 200 values, ensuring the limit applies to the bound set.

wgtmac

I haven't finished reviewing it yet, just post my finding so far. Please let me know what you think.

Handle constant expressions with tri-state binding detection and reject partially bound expressions. Bind unbound filters against the reader projection and pass case sensitivity explicitly from scan options. Disable floating-point bounds pruning while preserving null-count checks. Add regression coverage for finite bounds that omit NaNs, constant residuals, case sensitivity, and caller-provided read schemas.

YinZheng-Sun · 2026-10-08T07:26:31Z

Sorry for the late reply, and thanks for the detailed review. @wgtmac

YinZheng-Sun · 2026-10-09T12:34:31Z

I noticed a difference in how Iceberg-CPP and Java handle null comparisons.

Literal::operator<=> currently returns std::partial_ordering::unordered when either operand is null. Therefore, when key is null:

key < 100 // false
!(key < 100) // true
key >= 100 // false

However, RewriteNot rewrites NOT(key < 100) into key >= 100. These expressions are not equivalent under the current C++ semantics. Using the rewritten expression for row-group pruning can discard rows that the original expression would match in Evaluator.
Java’s literal comparator uses nullsFirst, under which both NOT(null < 100) and null >= 100 evaluate to false.
Was treating null as unordered an intentional semantic choice in C++. Should we preserve this behavior and make the rewrite null-aware, or adopt Java’s nullsFirst comparison semantics? @wgtmac Please let me know what you think. Thanks.

wgtmac

I think you're right that this indeed is a problem and different impls are not consistent:

impl	`x < v` on null	NOT
Java	true (`Literals` nulls-first)	rewritten in every pruning evaluator
Go	true (`nullsFirstCmp`)	rewritten in every pruning evaluator (`RewriteNotExpr`)
Rust	false	rewritten once in the scan; evaluator errors on a leftover `Not`
Python	false	metrics evaluator raises on a leftover `Not`

Perhaps in the short term (in this PR), we can keep RewriteNot in the pruning filter and make the evaluator use the same rule. Apply RewriteNot when building the Evaluator, or error on a leftover Not like iceberg-rust. Then not(key < 100) is key >= 100 for every value, so pruning keeps full strength on nullable columns.

In the long run, we can give the evaluator three states for null comparisons (null-first, null-last, unknown), so not(unknown) stays unknown. Pruning stays sound and semantics match SQL exactly.

WDYT? @YinZheng-Sun

YinZheng-Sun · 2026-10-10T03:57:30Z

I think you're right that this indeed is a problem and different impls are not consistent:

impl x < v on null NOT
Java true (Literals nulls-first) rewritten in every pruning evaluator
Go true (nullsFirstCmp) rewritten in every pruning evaluator (RewriteNotExpr)
Rust false rewritten once in the scan; evaluator errors on a leftover Not
Python false metrics evaluator raises on a leftover Not
Perhaps in the short term (in this PR), we can keep RewriteNot in the pruning filter and make the evaluator use the same rule. Apply RewriteNot when building the Evaluator, or error on a leftover Not like iceberg-rust. Then not(key < 100) is key >= 100 for every value, so pruning keeps full strength on nullable columns.

In the long run, we can give the evaluator three states for null comparisons (null-first, null-last, unknown), so not(unknown) stays unknown. Pruning stays sound and semantics match SQL exactly.

WDYT? @YinZheng-Sun

Thanks for the suggestion. I will apply the short-term approach firstly: Evaluator::Make() applies RewriteNot, so row evaluation and row-group pruning use the same normalized predicates.

Rewrite NOT after binding so nullable range comparisons use the same normalized predicates as row-group pruning. Add regression coverage for null values, comparison boundaries, and pruning nullable row groups.

advancedxy · 2026-10-10T06:36:16Z

 };

-class ICEBERG_EXPORT IsBoundVisitor : public ExpressionVisitor<bool> {
+class ICEBERG_EXPORT IsBoundVisitor : public ExpressionVisitor<std::optional<bool>> {


why this change?

I think whether an expression is bounded or not should be deterministic

The optional represents an internal traversal state, not uncertainty about whether an expression is bound. nullopt means that the subtree contains no references, as with True or False.
Constants need to be neutral when combining binding states: And(True, unbound) should be unbound, while And(True, bound) should be bound. Mixed bound and unbound predicates are still rejected. The public IsBound() API continues to return a definite boolean.
This follows Java’s IsBoundVisitor.

This is a good point. Let's make a separate pr of this fix? It's not part of the row group metrics filter but rather a prerequisite.
Also BTW, I think we should hide the IsBoundVisitor from the public contract, otherwise, we would be more careful about the iceberg_export APIs.

advancedxy · 2026-10-10T06:48:35Z

+
+  Result<bool> AlwaysFalse() override { return kRowsCannotMatch; }
+
+  Result<bool> Not(bool) override { return kRowsMightMatch; }


why is this?

it should be Not(bool result) override { return !result }?
If this is on purpose, then at least a comment should be added to justify.

I agree that we should add a comment for this workaround.

I think !result is a latent correctness bug.

The contract of this visitor is: return ROWS_CANNOT_MATCH only when it is proven that
no row in the group matches. ROWS_MIGHT_MATCH means "unknown", and negating "unknown"
is still "unknown", not "cannot match". So !result breaks the contract.

Example: stats x in [5, 15], filter NOT(x < 10). The child returns might-match, so
!result returns cannot-match and the group is skipped, although rows with
x in [10, 15] match. That is silent data loss.

advancedxy · 2026-10-10T07:05:23Z

+    if (!ref || !ref->type()->is_primitive() ||
+        MetadataColumns::IsMetadataColumn(ref->field_id()) ||
+        MetadataColumns::IsRowLineageColumn(ref->field_id())) {
+      return nullptr;
+    }


this seems unnecessary? if the ref column is nested, or meta column, then it should not be presented in the field_id_column_idx_ mapping.

That is true for complex-type containers and virtual metadata columns. However, nested primitive leaves do appear in the map. Physically stored _row_id and _last_updated_sequence_number can also appear there, since the map currently includes every physical leaf with a valid field ID.
Row-lineage nulls may be replaced with inherited values during reading, so their physical statistics do not necessarily describe the returned values. The null-reference guard is also needed for transform terms.
There is a redundant check here, though: IsMetadataColumn() already includes the row-lineage IDs. I’ll remove the separate IsRowLineageColumn() check.

advancedxy · 2026-10-10T07:24:35Z

+    if (!lower || lower->type()->type_id() != TypeId::kString) {
+      return kRowsMightMatch;
+    }


hmmm. this is strange. I think you are mixing validation and metrics evaluation here. I'm not sure it is good way to add such check here.

OK, I will remove it.

advancedxy · 2026-10-10T08:14:35Z

+      return kRowsMightMatch;
+    }
+    const auto& prefix = std::get<std::string>(value.value());
+    if (std::get<std::string>(lower->value()).compare(0, prefix.size(), prefix) > 0) {


I'm not sure we can use std::string to compare string columns directly. Per iceberg's spec String column must be encoded as UTF-8 bytes. we may compare unsigned utf-8 bytes directly. cc @wgtmac if you have more insights.

std::string::compare uses std::char_traits, which the standard specifies
compares as unsigned char, so this is an unsigned byte-wise comparison and matches
UTF-8 code point order.

advancedxy · 2026-10-10T08:33:13Z

+  }
+
+  template <typename Comparator>
+  bool VisitInequality(const std::shared_ptr<BoundReference>& ref, const Literal& value,


nit: VisitInequality is not a good name. There's no visit logic in the method.

The name follows iceberg-rust’s visit_inequality(), which serves the same purpose.

advancedxy · 2026-10-10T08:51:41Z

+    // Share a reader across adjacent row groups so Arrow can coalesce pre-buffered
+    // reads. Stop at gaps to keep physical row positions contiguous within batches.
+    while (end < context_->row_groups_.size() &&
+           context_->row_groups_[end].index == context_->row_groups_[end - 1].index + 1) {
+      row_groups.push_back(context_->row_groups_[end].index);
+      ++end;
+    }


why this change?

Seems like unrelated change to the metrics filter?

This change is needed when integrating row-group pruning with physical row-position tracking. Previously, the reader passed all selected row groups to one GetRecordBatchReader() call. Pruning can introduce gaps, so the new implementation splits the reader at those gaps to prevent a batch from spanning discontinuous file positions.
Keeping adjacent groups together preserves the existing multi-row-group batching and pre-buffering behavior, instead of creating a reader for every group. I’ll clarify this motivation in the comment.

advancedxy · 2026-10-10T09:13:12Z

+        context_->record_batch_reader_,
+        reader_->GetRecordBatchReader(row_groups, SelectedColumnIndices(projection_)));
+    context_->next_row_group_ = end;
+    metadata_context_.next_file_pos = context_->row_groups_[begin].first_row;


hmmm. It looks like that how iceberg-cpp currently support _pos metadata column is problematic. It tracks next_file_pos in MetadataColumnContext and updates/incrs per batch. This might work for parquet reader without data skipping.. Once row group filtering, and later page skipping is kicked in, it would be hard to track that correctly. I think we should follow how parquet-rs: apache/arrow-rs#8715 or iceberg-java's approach(org.apache.iceberg.parquet.ParquetValueReaders.PositionReader) to support _pos metadata column. WDYT @wgtmac

Yes, this is a new issue if data skipping is supported. I think either of your proposed approach is fine but we would wait longer if we want to fix it in Arrow C++.

Let me see if whether we can add the support in iceberg-cpp first, then we can contribute back to arrow c++.

Merge origin/main while retaining nested nullability and unknown-type predicate tests. Return the negated result from the row group metrics NOT visitor, with RewriteNot still applied in Make().

…ter-reader

advancedxy · 2026-10-12T03:25:28Z


+namespace {
+
+Result<std::optional<bool>> CombineResults(const std::optional<bool>& left_result,


This method should be scoped to IsBoundVisitor? CombineBoundResults or make it a private helper method of IsBoundVisitor?

advancedxy · 2026-10-12T03:26:52Z

I did a quick pass on the new code and responses. I think most of them are addressed and will do a more detailed round later.

yinzhengsun added 5 commits September 21, 2026 20:05

feat(parquet): add statistics-based row group filtering

84415a5

refactor

64cc97c

fix ci

3938983

fix ci

782b503

fix

a1211cc

wgtmac requested changes Sep 24, 2026

View reviewed changes

Comment thread src/iceberg/data/file_scan_task_reader.cc Outdated

Comment thread src/iceberg/parquet/parquet_metrics_row_group_filter.cc

Comment thread src/iceberg/file_reader.h Outdated

Comment thread src/iceberg/data/file_scan_task_reader.cc Outdated

Fix binder

7edfe87

YinZheng-Sun requested a review from wgtmac October 9, 2026 07:09

fix: account for nullable ancestors when binding null predicates

90d07e2

YinZheng-Sun commented Oct 9, 2026

View reviewed changes

Comment thread src/iceberg/expression/predicate.cc

yinzhengsun added 2 commits October 9, 2026 16:41

Fix

4c6acc5

Fix

3f67e95

wgtmac requested changes Oct 9, 2026

View reviewed changes

fix(expression): normalize NOT when creating evaluators

f7d9db7

Rewrite NOT after binding so nullable range comparisons use the same normalized predicates as row-group pruning. Add regression coverage for null values, comparison boundaries, and pruning nullable row groups.

advancedxy reviewed Oct 10, 2026

View reviewed changes

yinzhengsun added 3 commits October 10, 2026 18:43

Fix

d7d8cdd

fix(parquet): resolve main conflicts and align NOT visitor

6001481

Merge origin/main while retaining nested nullability and unknown-type predicate tests. Return the negated result from the row group metrics NOT visitor, with RewriteNot still applied in Make().

fix

aca3b9e

YinZheng-Sun force-pushed the parquet-row-group-filter-reader branch from 7e379e7 to aca3b9e Compare October 11, 2026 14:00

YinZheng-Sun requested review from advancedxy and wgtmac October 11, 2026 14:00

yinzhengsun added 2 commits October 11, 2026 23:11

Merge remote-tracking branch 'origin/main' into parquet-row-group-fil…

25428cb

…ter-reader

fix(test): include data_table_scan.h after TableScan split

767f701

advancedxy reviewed Oct 12, 2026

View reviewed changes


		Result<bool> AlwaysFalse() override { return kRowsCannotMatch; }

		Result<bool> Not(bool) override { return kRowsMightMatch; }


		namespace {

		Result<std::optional<bool>> CombineResults(const std::optional<bool>& left_result,

Conversation

YinZheng-Sun commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Behavior change

Testing

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

YinZheng-Sun commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

YinZheng-Sun commented Oct 9, 2026

Uh oh!

wgtmac left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YinZheng-Sun commented Oct 10, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YinZheng-Sun Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

advancedxy commented Oct 12, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

YinZheng-Sun commented Sep 22, 2026 •

edited

Loading

YinZheng-Sun commented Oct 8, 2026 •

edited

Loading

wgtmac left a comment •

edited

Loading

YinZheng-Sun Oct 11, 2026 •

edited

Loading