Skip to content

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

Open
advancedxy wants to merge 2 commits into
apache:mainfrom
advancedxy:feat/parquet-row-number-reader
Open

advancedxy wants to merge 2 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.

This branch has not been deployed

No deployments
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.

1 participant