Skip to content

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

Merged
wgtmac merged 3 commits into
apache:mainfrom
advancedxy:feat/parquet-row-number-reader
Oct 15, 2026
Merged

wgtmac merged 3 commits into
apache:mainfrom
advancedxy:feat/parquet-row-number-reader

Conversation

@advancedxy

Copy link
Copy Markdown
Contributor

What

Add a RowNumberReader, modeled after parquet-rs, that emits absolute file row positions from the selected Parquet row groups.

Use the resulting position array for _pos and inherited _row_id in record-batch projection. Preserve physical lineage values and cross-row-group batching. Restore consumed ranges if array construction fails so the batch can be retried.

Why

Follow-up to the row-position tracking discussion in #956.

Updating next_file_pos by the emitted batch size assumes contiguous file rows. Row-group pruning can introduce gaps, so each selected row must retain its original position in the file.

Testing

  • parquet_test, arrow_test, and avro_test passed.
  • Coverage includes skipped/reordered row groups, batches spanning row groups, physical and inherited lineage, and allocation-failure recovery.
  • Pre-commit and git diff --check passed.

Limitations and follow-up

The Iceberg-side RowNumberReader can account for row-group-level skipping only: it knows the selected row groups and their row counts. It cannot account for page-level skipping inside Arrow C++ because the skipped row ranges are not exposed alongside the output batches; batch lengths alone are insufficient to recover the original positions.

Supporting page-level skipping requires integrating row-number generation into Arrow C++'s Parquet reader, where it can stay synchronized with page skipping and the actual rows emitted. This is the planned follow-up. Iceberg C++ can then consume that position array for _pos and inherited _row_id, replacing its local RowNumberReader. Keeping position generation separate from Iceberg metadata semantics prepares for that integration.

Comment on lines +167 to +172
// Metadata columns belong to top-level rows; nested fields need no positions.
ICEBERG_ASSIGN_OR_RAISE(
projected_array,
ProjectNestedArray(parquet_array, output_arrow_type, nested_type,
field_projection.children, metadata_context, pool));
field_projection.children, metadata_context,
/*row_positions=*/nullptr, pool));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not satisfied about these nullptr changes yet, however I don't find a more elegant way to passing row positions as it will touch all the ProjectedXXXMethods.

happy to take suggestions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A cleaner option would be to keep row positions only in a top-level struct projection helper, and use a separate ProjectNestedArray overload without the row_positions parameter for nested fields.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, let me address this

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me just merge this PR. You can create followup PR if you wish :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can create followup PR if you wish :)

thanks. Let me create a follow-up pr then.

@wgtmac
wgtmac merged commit 5a93503 into apache:main Oct 15, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants