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.

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.

Good call. Thanks for digging into the issue.

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.

advancedxy · 2026-10-12T08:33:24Z

I did another round, and I think the metrics filter itself is in good shape. The changes to parquet_reader.cc is correct but it do add more complexity and don't work well with future page skipping support. For that, I have discussed with agent to introduce RowPositionReader in the iceberg-cpp side first, then make it to the upstream arrrow cpp's parquet reader, which we would also make patches to support page skipping. Therefore, I think we can split this PR into 3-4 smaller ones:

binder's fix about IsBoundVisitor, which I mentioned
fix: respect ancestor nullability when folding IS NULL/NOT NULL predicates #995 for nullability fix
The metrics filter in this pr
The integration part of all the previous prs(and probably after the introduce of RowPositionReader, which I may submit a PR later today).

WDYT @YinZheng-Sun @wgtmac

wgtmac · 2026-10-12T14:07:45Z

That sounds good to me @advancedxy

wgtmac · 2026-10-12T16:25:05Z

                                                   std::shared_ptr<Expression> unbound,
                                                   bool case_sensitive) {
  ICEBERG_ASSIGN_OR_RAISE(auto bound_expr, Binder::Bind(schema, unbound, case_sensitive));
+  ICEBERG_ASSIGN_OR_RAISE(bound_expr, RewriteNot::Visit(bound_expr));


These two evaluators now disagree on the same row.

Suppose a file uses identity(key) partitioning. Its partition value is null, and it contains this row:

key value

null 7

The filter is NOT(key < 100) OR value = 5.

Evaluator rewrites the filter to key >= 100 OR value = 5. Both checks return false for this row, so the row does not match.

ResidualEvaluator still checks the original filter:

key < 100 returns false for the null partition value.

NOT(false) becomes true.

true OR value = 5 becomes true for the whole file.

This drops the value = 5 condition. If file statistics cannot reject the file, the reader gets a true residual. Any later filter using that residual will keep the row, even though Evaluator rejects it.

Good catch, thanks!！

…quet-row-group-filter-reader

wgtmac · 2026-10-13T09:57:57Z

So now this pr is ready for review, right? @YinZheng-Sun @advancedxy

advancedxy · 2026-10-13T10:36:03Z

So now this pr is ready for review, right? @YinZheng-Sun @advancedxy

I'm submitting the row number reader now. I think we can rebase this pr on that.

YinZheng-Sun · 2026-10-13T11:13:13Z

So now this pr is ready for review, right? @YinZheng-Sun @advancedxy

I'm submitting the row number reader now. I think we can rebase this pr on that.

How about decoupling them so this PR doesn't need to wait

In this PR, keep only the statistics-based row group filter (parquet_metrics_row_group_filter.*) with its unit tests. It's self-contained and doesn't touch the read path, so it can be reviewed and merged independently.
Once the RowPositionReader lands, A follow-up PR to apply the filter in the Parquet reader on top of it. That way _pos comes from the row position reader instead of the manual counter, and it also fits better with the future page skipping work. WDYT@advancedxy @wgtmac

advancedxy · 2026-10-13T11:20:19Z

In this PR, keep only the statistics-based row group filter (parquet_metrics_row_group_filter.*) with its unit tests. It's self-contained and doesn't touch the read path, so it can be reviewed and merged independently.

this sounds good to me.

advancedxy · 2026-10-13T11:48:17Z

I'm submitting the row number reader now. I think we can rebase this pr on that.

fyi: #1006

The lower-only check is subsumed by the [lower, upper] range check, which is equivalent to Java's two-step filtering.

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 Outdated

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

YinZheng-Sun mentioned this pull request Oct 12, 2026

fix(expression): treat constants as neutral in IsBoundVisitor #1004

Merged

wgtmac reviewed Oct 12, 2026

View reviewed changes

fix comments

a2e276c

YinZheng-Sun force-pushed the parquet-row-group-filter-reader branch from 204399b to a2e276c Compare October 13, 2026 03:03

Merge branch 'main' of https://github.com/apache/iceberg-cpp into par…

c4e2f28

…quet-row-group-filter-reader

advancedxy mentioned this pull request Oct 13, 2026

feat(parquet): add row number reader for selected row groups #1006

Open

refactor(parquet): simplify IN row group pruning

b34427d

The lower-only check is subsumed by the [lower, upper] range check, which is equivalent to Java's two-step filtering.


		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!

Choose a reason for hiding this comment

Uh oh!

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!

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!

advancedxy commented Oct 12, 2026

Uh oh!

wgtmac commented Oct 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

YinZheng-Sun commented Oct 13, 2026 •

edited

Loading