fix(plugin-iceberg, native): Fix wrong results when reading row lineage columns

[Joe-Abraham]

Description

A row lineage column (_row_id, _last_updated_sequence_number) is
either stored in the data file or, when the stored value is null,
inherited: _row_id from the file's first_row_id plus the row
position, _last_updated_sequence_number from the file's data sequence
number. A file rewritten by compaction or by a row-preserving
UPDATE/MERGE can hold both kinds of rows, and file statistics describe
only the stored values.

• Split pruning on _last_updated_sequence_number keeps a file when
  either its stored-value bounds or its data sequence number can match.
  A file without lineage bounds is pruned when the predicate excludes
  every value up to its file sequence number, since inherited values are
  its data sequence number and stored values are copied from rows of
  earlier commits. Otherwise it is kept when its counts show stored
  values. A file with no lineage metrics is pruned on its data sequence
  number only while the APPEND snapshot that added it is retained,
  since appended files store no lineage values.
• The Parquet and ORC readers no longer apply row lineage predicates to
  row group and stripe statistics.
• Statistics for the equality delete scan strip metadata and row
  lineage columns from the predicate before planning delete files, as
  the data table statistics already do, since Iceberg cannot bind them
  against the table schema.
• On the native worker, the Iceberg connector adds row lineage columns
  that appear only in the scan's assignments to the table handle's data
  columns, so the reader returns their stored values. A new
  PrestoToVeloxConnector::toVeloxTableHandle overload receives the
  TableScanNode assignments; the default implementation ignores them.

The Velox side of the native fix, applying row lineage filters after
inherited values are synthesized, is in facebookincubator/velox#18435.
The pinned Velox commit already includes it.

Motivation and Context

Queries filtering on a row lineage column could silently drop rows:

• In a file holding both stored and inherited values, split pruning
  evaluated only the stored-value bounds, and Parquet and ORC
  statistics saw only stored values, so rows with inherited values
  were skipped.
• A compacted file written with counts or none metrics for the
  lineage columns was pruned by its own, newer data sequence number,
  although its rows store older values.

On the native worker, a query that projected a row lineage column
without filtering on it returned inherited values for rows with stored
values, so it disagreed with the same query filtered on that column.

Impact

No public API or configuration changes. Queries filtering on row
lineage columns return correct results. Row lineage predicates no
longer prune Parquet row groups or ORC stripes. A file without lineage
metrics whose APPEND snapshot has expired is pruned only when the
predicate excludes every value up to the file's sequence number, as in
_last_updated_sequence_number > N, so other predicates can read more
data.

Pruning a file without lineage metrics on its data sequence number
assumes that writers store lineage values only in commits that rewrite
rows, which Iceberg reports as OVERWRITE or REPLACE rather than APPEND.
The Iceberg spec does not guarantee this. Pruning on the file sequence
number follows from the spec's rules for copying lineage values.

Before this change, statistics for the equality delete scan failed to
compute when a query filtered on a row lineage column. With
ignore_stats_calculator_failures at its default of true, such
queries were planned without those statistics rather than failing.

Test Plan

• TestIcebergRowLineageBase: moved the predicate tests from
  TestIcebergRowLineage into the base class and parameterized them by
  pushdown_filter_enabled. Added cases for files mixing stored and
  inherited values (Parquet and ORC), _row_id predicates on stored and
  inherited values, IS NULL, deleted rows, lineage predicates combined
  with regular columns, compacted files with counts and none lineage
  metrics, appended files without metrics, files whose APPEND snapshot
  has expired (pruned only by predicates above their file sequence
  number), and projection-only versus filtered queries.
• TestIcebergUtil#testLastUpdatedSequenceNumberSplitPruning: split
  pruning across combinations of lineage bounds, value and null counts,
  first_row_id, file sequence number, and APPEND snapshot retention.
• PrestoToVeloxConnectorTest: projected row lineage columns reach the
  data columns with their field IDs without becoming filter columns, and
  a column in both a predicate and the assignments is added once.
• TestIcebergV3RowLineage (presto-native-tests): runs the base suite
  on native workers with pushdown_filter_enabled true and false, and
  compares each query with the Java workers.

Contributor checklist

• Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
• PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
• Documented new properties (with its default value), SQL syntax, functions, or other functionality.
• If release notes are required, they follow the release notes guidelines.
• Adequate tests were added if applicable.
• CI passed.
• If adding new dependencies, verified they have an OpenSSF Scorecard score of 5.0 or higher (or obtained explicit TSC approval for lower scores).

Release Notes

== RELEASE NOTES ==

Iceberg Connector Changes
* Fix queries filtering on ``_row_id`` or ``_last_updated_sequence_number`` skipping rows whose values are inherited or stored in compacted files without lineage column bounds.

Prestissimo (Native Execution) Changes
* Fix Iceberg ``_row_id`` and ``_last_updated_sequence_number`` returning inherited instead of stored values when the query does not filter on them.

Summary by Sourcery

Correct Iceberg row lineage reads and pruning across Java and native execution paths.

Bug Fixes:

• Fix Iceberg queries filtering on row lineage columns so inherited values and stored values in rewritten files are not incorrectly skipped.
• Fix native Iceberg scans to return physically stored row lineage values when they are projected without filters.
• Prevent row lineage predicates from causing equality-delete statistics planning failures.

Enhancements:

• Improve lineage-aware split pruning using stored bounds, value counts, file sequence numbers, and retained append snapshots while avoiding row-group and stripe pruning based on incomplete lineage statistics.
• Pass scan assignments through native connector table-handle conversion so projected lineage columns are included in the data scan.

Documentation:

• Update Iceberg connector documentation for corrected row lineage behavior.

Tests:

• Expand Java and native Iceberg row lineage coverage across stored and inherited values, compaction, missing metrics, Parquet and ORC files, deletes, projections, and predicate combinations.

[sourcery-ai]

Reviewer's Guide

Fixes Iceberg V3 row lineage filtering and projection by propagating scan output columns to pushdown, preserving stored lineage values in native reads, making file/split pruning conservative for inherited or incomplete metrics, and adding broad Java/native end-to-end coverage across pushdown modes.

Sequence diagram for Iceberg row lineage filter pushdown

sequenceDiagram
    participant Planner
    participant IcebergFilterPushdown
    participant NativeConnector
    participant VeloxReader
    Planner->>IcebergFilterPushdown: pushdownFilter(filter, desiredColumns, currentLayoutHandle)
    IcebergFilterPushdown->>NativeConnector: getConnectorPushdownFilterResult(desiredColumns)
    NativeConnector->>VeloxReader: toVeloxTableHandle(requestedColumns)
    VeloxReader-->>NativeConnector: Read stored lineage values
    NativeConnector-->>Planner: Pushdown result with lineage projection

Loading

Flow diagram for conservative Iceberg lineage split pruning

flowchart TD
    A[Lineage constraint on _last_updated_sequence_number] --> B{Stored bounds available?}
    B -->|Yes| C[Evaluate bounds and inherited data sequence number]
    B -->|No| D{Lineage value counts indicate stored values?}
    D -->|Yes| E[Keep file]
    D -->|No| F{File may inherit data sequence number?}
    F -->|Yes| E
    F -->|No| G[Evaluate data sequence number and nullability]

Loading

File-Level Changes

Change	Details	Files
Propagate table-scan output columns through connector filter pushdown so projection-only row lineage columns are retained.	Add desired column sets to the pushdown API and callers. Use desired columns instead of the initially empty layout request when constructing Iceberg requested columns. Update Hive test plumbing and implementations for the new parameter.	presto-hive-common/src/main/java/com/facebook/presto/hive/rule/BaseSubfieldExtractionRewriter.java presto-hive/src/main/java/com/facebook/presto/hive/rule/HiveFilterPushdown.java presto-hive/src/test/java/com/facebook/presto/hive/AbstractTestHiveClient.java presto-iceberg/src/main/java/com/facebook/presto/iceberg/optimizer/IcebergFilterPushdown.java
Ensure native Iceberg readers receive projection-only lineage handles and return stored physical values.	Merge requested row lineage columns into the native table handle's final data columns. Preserve lineage fields needed by Velox even when they are not filter columns.	presto-native-execution/presto_cpp/main/connectors/IcebergPrestoToVeloxConnector.cpp
Correct Iceberg file and split pruning for stored, inherited, mixed, and metadata-poor sequence lineage values.	Exclude row lineage predicates from Parquet and ORC stored-statistics predicates. Account for inherited data sequence numbers alongside stored bounds and value counts. Keep files with lineage values or uncertain metrics, including append-derived files, and memoize append snapshot sequence numbers. Strip lineage constraints from delete-file scans.	presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergPageSourceProvider.java presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergSplitSource.java presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergUtil.java presto-iceberg/src/main/java/com/facebook/presto/iceberg/procedure/splits/RewriteDataFilesIcebergSplitSource.java
Expand row lineage end-to-end coverage across Java and native execution modes.	Centralize tests for stored, inherited, and mixed lineage, compaction, V2 behavior, equality deletes, combined predicates, and split pruning. Run the shared suite with pushdown disabled on Java and enabled or disabled on native workers. Compare native query results against a Java expected runner while validating split-pruning behavior locally.	presto-iceberg/src/test/java/com/facebook/presto/iceberg/TestIcebergRowLineageBase.java presto-iceberg/src/test/java/com/facebook/presto/iceberg/TestIcebergRowLineage.java presto-native-tests/src/test/java/com/facebook/presto/nativetests/iceberg/TestIcebergV3RowLineage.java

Possibly linked issues

• native(Iceberg): Implement support for reading row lineage tracking columns added in V3 #27197: The PR directly fixes filtering, projection, split pruning, and stored-value handling for Iceberg V3 row lineage columns.
• #native(Iceberg): PR directly implements and validates correct reading, filtering, projection, inheritance, and pruning of Iceberg V3 lineage columns.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've found 2 issues

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergUtil.java" line_range="877" />
<code_context>
+        if (valueCount != null && (nullCount == null || nullCount < valueCount)) {
+            return true;
+        }
         return domain.includesNullableValue(dataSequenceNumber);
     }

</code_context>
<issue_to_address>
**Stored lineage rows disappear**

When a V3 file physically stores `_last_updated_sequence_number` values but omits that column’s bounds and counts, `lastUpdatedSequenceNumberMatches` falls back to `dataSequenceNumber` and prunes files whose stored values match the filter, so query results omit valid rows.

Treat missing lineage metrics as unknown and avoid pruning on `dataSequenceNumber` when stored lineage values may be present.
</issue_to_address>

### Comment 2
<location path="presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergPageSourceProvider.java" line_range="478-479" />
<code_context>
         ImmutableMap.Builder<ColumnDescriptor, Domain> predicate = ImmutableMap.builder();
         effectivePredicate.getDomains().get().forEach((columnHandle, domain) -> {
+            // Stored row lineage values exclude inherited ones, so their statistics cannot prune.
+            if (columnHandle.isRowIdColumn() || columnHandle.isLastUpdatedSequenceNumberColumn()) {
+                return;
+            }
             String baseType = columnHandle.getType().getTypeSignature().getBase();
</code_context>
<issue_to_address>
**Inherited row IDs get pruned**

When a file contains both stored row IDs and null row IDs that inherit positional values, and the predicate matches an inherited value outside the stored values’ bounds, for a mixed-lineage file, `IcebergSplitManager` still applies the row-ID constraint through Iceberg’s ordinary metrics evaluator before the page source runs. That evaluator sees only the stored row-ID bounds, so it drops the file when those bounds exclude a matching inherited ID, and the query omits valid rows.

Remove row-ID constraints from the Iceberg scan filter and evaluate them with lineage-aware split logic, or otherwise make scan pruning account for inherited row IDs.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 1 · 2

Open (3)

• Constructing a std::shared_ptr directly from toVeloxColumnHandle(&entry, typeParser) is risky… · New
• requestedColumns is now always Optional.of(...), which can change semantics vs the previous… · New
• This manual try/finally close pattern can mask the original exception if writer.close() throws… · New

Resolved since last review (1)

• assertQueryOrdered(...) is not available on AbstractTestQueryFramework, so this test won't compile.…

[sourcery-ai]

Hey - I've found 1 issue

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergUtil.java" line_range="874-877" />
<code_context>
+        if (getMetric(file.lowerBounds(), fieldId) != null && getMetric(file.upperBounds(), fieldId) != null) {
+            return evaluator.eval(file) || (mayInherit && domain.includesNullableValue(dataSequenceNumber));
+        }
+        if (valueCount != null && (nullCount == null || nullCount < valueCount)) {
+            return true;
+        }
         return domain.includesNullableValue(dataSequenceNumber);
     }

</code_context>
<issue_to_address>
**Matching rows disappear**

When a data file stores lineage values but has no bounds or value-count metric, and the predicate matches a stored value but not the file’s data sequence number, `lastUpdatedSequenceNumberMatches` falls back to matching only the file’s data sequence number, prunes the file, and omits matching rows from query results.

Treat files with missing lineage bounds and value counts as potentially matching stored values instead of relying only on the data sequence number.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

[sourcery-ai]

Hey - I've found 2 issues

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergUtil.java" line_range="880-881" />
<code_context>
+        if (valueCount != null && (nullCount == null || nullCount < valueCount)) {
+            return true;
+        }
+        if (valueCount == null && !appendSequenceNumbers.get().contains(file.fileSequenceNumber())) {
+            return true;
+        }
         return domain.includesNullableValue(dataSequenceNumber);
</code_context>
<issue_to_address>
**Matching rows are pruned**

When an APPEND file physically stores `_last_updated_sequence_number` values but lacks value-count metrics or bounds, and a predicate matches a stored value but not the file's data sequence number, `lastUpdatedSequenceNumberMatches` treats the missing metrics as proof that no values are stored, then tests only `dataSequenceNumber`; split pruning drops the file and matching rows disappear.

Treat absent metrics as unknown for APPEND files and avoid pruning based only on `dataSequenceNumber` when stored lineage values may exist.
</issue_to_address>

### Comment 2
<location path="presto-hive-common/src/main/java/com/facebook/presto/hive/rule/BaseSubfieldExtractionRewriter.java" line_range="185" />
<code_context>
                 metadata,
                 handle.getConnectorHandle(),
                 replacedExpression,
+                ImmutableSet.copyOf(tableScan.getAssignments().values()),
                 handle.getLayout());

</code_context>
<issue_to_address>
**Projection-only lineage columns stay missing**

When a table-scan rewrite has a current layout whose `requestedColumns` is empty or stale and the filter is `TRUE_CONSTANT`, `pushdownFilter` returns the existing layout on its `TRUE_CONSTANT` fast path before forwarding `desiredColumns`; the native connector then has no requested lineage columns to add to its data columns, so projection-only queries can return inherited values instead of stored values.

Update the existing layout with the scan's `desiredColumns` on the true-filter fast path before returning it.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

[sourcery-ai]

Hey - I've reviewed your changes and they look great!

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

[sourcery-ai]

Hey - I've reviewed your changes and they look great!

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

[nmahadevuni]

Thanks @Joe-Abraham

Suggestions

1. The requestedColumns change is broader than the fix needs (IcebergFilterPushdown.java:156). Every native Iceberg scan with pushdown enabled now:

   • sends the full set of scan column handles in the layout JSON;
   • includes those columns in IcebergTableLayoutHandle.equals/hashCode, so layouts that differ only by projection are no longer equal;
   • can carry stale contents, because PruneUnreferencedOutputs can drop assignments after pushdown. On native this only puts an unused lineage column into dataColumns, which is harmless but misleading.

   Meanwhile only lineage columns are read from it. Options:

   • (a) set requestedColumns to just the lineage columns among desiredColumns, which keeps the layout small and the intent explicit;
   • (b) at least document on getRequestedColumns() that, on the pushdown path, t pushdown time.

   It's also worth asking whether the native plan converter could take this froments directly. That would avoid touching the abstract method inpresto-hive-common.

2. ORC is changed but not tested. The predicateExcludeTimeType change in the ORCvered: TestIcebergRowLineage and the native test both use PARQUET only. Either add an ORC variant (if Java ORC reads stored lineage values at all) or say that ORC is out of scope.

3. No test for the APPEND-without-metrics path. The none metrics test only cover the file). Nothing exercises the branch where a file with no lineage metrics wasadded by an APPEND and can be pruned by dataSequenceNumber. That branch is the one that relies on the "an append writes only new rows" assumption, so it's the one most worth locking
   down. A table with write.metadata.metrics.default=none plus an append, and an, would cover it.

4. The APPEND assumption should be written down more precisely. In Iceberg, BaseRowDelta.operation() returns APPEND when a RowDelta adds data files only. That is still "new rows only", so
   the assumption holds. But it's an assumption about the writer's behaviour, nontees. Expand the comment in lastUpdatedSequenceNumberMatches to name theoperations it relies on, so a future reader can check it.

5. Stale comment on getNonMetadataColumnConstraints (IcebergUtil.java:804). It says lineage columns are "evaluated at the split level instead". In practice only_last_updated_sequence_number is pruned at split level. _row_id has no split pruning and is filtered only per row. That's correct and conservative, but the comment should say so.

Nits

• The comment in BaseSubfieldExtractionRewriter (// 'desiredColumns' are the table scan's output columns; ...) was cut down a lot in HEAD. The earlier version explained why
  currentLayoutHandle can't be used (pushdown connectors skip the legacy getTabl, which is the useful part. Consider keeping one sentence of it.
• Two names in lastUpdatedSequenceNumberMatches could be clearer: mayInherit → mayHaveInheritedValues, and the generic getMetric helper → getColumnMetric (or inline it).
• IcebergSplitSource's public constructor now takes a Table. Only RewriteDataFilit, so this is fine, but requireNonNull(table, ...) only runs in the privateconstructor. That's consistent with the existing code.

Questions

1. Is the stored-value behaviour for ORC files supported in Velox and in the Java ORC reader? If not, is skipping the stripe statistics for ORC only a defensive change?
2. Did you consider getting projection-only lineage columns from the native TableScanNode assignments instead of the layout handle?
3. Add a release note. Queries that filter on lineage columns could previously return wrong results, which is user-visible.

Testing Recommendations

• APPEND-added file with metrics mode none: check it is pruned, and that the results are correct.
• APPEND snapshot expired after a rewrite: check the file is kept and the result
• A delete-file case where the filter includes a lineage predicate, to cover the getDeleteFiles change. Equality deletes plus _row_id = X would do.
• An ORC variant of the mixed stored/inherited test, if ORC is in scope.

[Joe-Abraham]

Thanks @nmahadevuni.

1. Reverted the desiredColumns/requestedColumns change; hive-common/hive/IcebergFilterPushdown match master. Native now gets projection-only lineage columns from the TableScanNode assignments via a new toVeloxTableHandle(tableHandle,
   assignments, …) overload, so the layout handle is unchanged and nothing goes stale.
2. ORC is in scope: the mixed stored/inherited test runs on Parquet and ORC, Java and native, and fails without the stripe-stats exclusion.
3. testAppendedFileWithoutMetricsIsPruned covers metrics none plus appends.
4. The comment now names the operations it relies on (AppendFiles; a RowDelta/OverwriteFiles that only adds data files) and calls it a writer assumption.
5. Fixed: only _last_updated_sequence_number prunes splits; _row_id is filtered per row.
    
   Nits: renames done; the rewriter comment went away with the revert.
    
   Questions: yes, both the Java ORC reader and Velox return stored values. Yes, the native path now uses the assignments.
    
   Testing: added an expired-APPEND test. The equality-delete test now runs with AUTOMATIC join distribution and stats failures enabled, and fails without the getDeleteFiles change.

[nmahadevuni]

use dynamic_cast and VELOX_CHECK_NOT_NULL, like the one at line 637

[nmahadevuni]

lets add _last_updated_sequence_number test too. And also case where lineage column is in projection and filter.

[aditi-pandit]

Thanks @Joe-Abraham. Moving the native side to the TableScanNode assignments is much cleaner than threading desiredColumns through hive-common. I checked the pinned Velox submodule (3f23f2386) and it already contains facebookincubator/velox#18435 (6358057b8, "Apply row lineage filters after synthesis"), so this PR has no hidden Velox dependency.

Just some test simplification suggestions.

1. [Important] PR description and commits are out of date

• The Description and Impact sections still say the scan columns are passed to getConnectorPushdownFilterResult as desiredColumns, and that BaseSubfieldExtractionRewriter#getConnectorPushdownFilterResult gains a parameter. That change was reverted in 4613aa6. Please describe the new toVeloxTableHandle(tableHandle, assignments, ...) overload instead.
• The Test Plan says TestIcebergRowLineage passes 18/18. The test list has changed since then, so please refresh that count.
• The branch has 4 commits, including "Review comments" and a refactor that reverts part of the first commit. Please squash them into one commit with the PR title before merge.

2. [Important] Pruning on _last_updated_sequence_number now depends on snapshot retention

IcebergUtil.java:885: a data file that does not physically contain the lineage column has no metrics for it (valueCount == null). That covers every file a plain append writes, whatever the metrics mode. Before this PR those files were pruned by dataSequenceNumber. Now they are pruned only if the snapshot that added them is a live APPEND. So these are always scanned:

• every appended file once its snapshot has been expired. That happens on any table that runs expire_snapshots, which in practice is most of the files in a production table;
• files added by an overwrite commit that writes no lineage, for example Presto's own UPDATE/MERGE (RowDelta with deletes);
• files added by a replace commit from a compaction that does not preserve lineage. That includes Presto's own rewrite_data_files: as the TestIcebergRowLineageBase javadoc notes, Presto cannot write stored lineage values, so every Presto-compacted file falls into this bucket.

The result is still correct, and testFileFromExpiredAppendIsNotPruned locks in the conservative behaviour on purpose. Still, this is a real regression for the main use case of this predicate (incremental reads like WHERE _last_updated_sequence_number > N on a long-lived table). Could you:

• say in the code comment that pruning stops for files whose APPEND snapshot has expired, so nobody reads that as a bug later; and
• open a follow-up issue for a stronger signal. One option: the field IDs present in the file. The coordinator cannot see them without reading the footer, but the worker can skip a row group or the whole file once it knows the column is absent and the inherited dataSequenceNumber does not match.

3. [Suggestion] Add IcebergColumnHandle.isRowLineageColumn()

isRowIdColumn() || isLastUpdatedSequenceNumberColumn() now shows up in three places: IcebergUtil.java:807, IcebergPageSourceProvider.java:478 and IcebergPageSourceProvider.java:631-632. A single helper next to isRowIdColumn() would keep these in sync if another lineage column is ever added, matching what kRowLineageColumnNames already does on the C++ side.

4. [Suggestion] Factor out the column-handle conversion in toVeloxTableHandle

IcebergPrestoToVeloxConnector.cpp:710-725: the new loop repeats the "dedupe by name, toVeloxColumnHandle, shared_ptr, dynamic_pointer_cast" block used by the partition-column and predicate-column loops above it. A small lambda such as addColumnHandle(const protocol::iceberg::IcebergColumnHandle&) would turn all three loops into two or three lines each.

5. [Suggestion] Unit test for lastUpdatedSequenceNumberMatches

Every branch of this function is only reached through end-to-end tests that build files with the Iceberg API, and some branches are not reached at all:

• bounds present but nullCount == null (mayHaveInheritedValues true from a missing count);
• valueCount present, nullCount == null;
• valueCount == nullCount (all stored values null, so the file falls back to dataSequenceNumber);
• fileSequenceNumber() == null.

A table-driven test in TestIcebergUtil with a stub DataFile (or DataFiles.builder(...).withMetrics(...)) would cover all of them in milliseconds and would document the decision table better than the comment does.

6. [Nit] C++ test setup is copied three times

icebergProjectedRowLineageColumns, icebergAssignmentsRejectNonIcebergColumn and icebergFilteredAndProjectedRowLineageColumn each rebuild the IcebergTableHandle, the same tableSchemaJson literal (already used in icebergTableSchemaFieldIds), the TableHandle and a single-entry assignments map. A helper like makeIcebergTableHandle(layout) plus a lineage column-handle factory would cut about 60 lines. Also, icebergFilteredAndProjectedRowLineageColumn sits after the anonymous namespace near line 431, away from the other two. Moving it next to them, or moving createSingleRangeDomain up, would keep the lineage tests together.

7. [Nit] /*assignments=*/{} relies on name hiding

IcebergPrestoToVeloxConnector.cpp:666-667: {} resolves to the protocol::Map overload only because IcebergPrestoToVeloxConnector hides the base IndexHandle overload. If Iceberg ever overrides the index overload, this call becomes ambiguous. Passing a named empty map (or protocol::Map<...>{}) avoids that.

Questions

1. Since Presto's rewrite_data_files does not carry stored lineage into the compacted files, _row_id and _last_updated_sequence_number reset after a Presto compaction. That is out of scope here. Is there an issue tracking it?
2. Was TestIcebergV3RowLineage run on ORC with pushdown_filter_enabled=true? The PR body only says the Java suite passed.

[prestodb-ci]

AI session: claude/1f5789c9-7109-427b-9e36-2234bea4c0ed

The AI reviewer completed the code review and posted the findings in the pull request review timeline.

[prestodb-ci]

No actionable findings found.

[Joe-Abraham]

@sourcery-ai review

[sourcery-ai]

Sorry @Joe-Abraham, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 32 minutes by commenting @sourcery-ai review. Upgrade to get a review now.

[steveburnett]

LGTM! (docs)

Pull branch, local doc build, looks good. Thanks for the doc!