Skip to content

test(parquet): show push decoder needs a whole row group before decoding a batch - #11218

Merged
alamb merged 4 commits into
apache:mainfrom
alamb:test/push-decoder-partial-page-push
Sep 28, 2026
Merged

alamb merged 4 commits into
apache:mainfrom
alamb:test/push-decoder-partial-page-push

Conversation

@alamb

@alamb alamb commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

While discussing #6946 with @adriangb, a question came up about whether ParquetPushDecoder would decode a batch if only the pages needed for that batch had been pushed or if it insists on having all pages for the row group first.

This PR adds a test that answers that question

For the current code, the ParquetPushDecoder requires that all ranges for the row group are present

What changes are included in this PR?

test_decoder_first_page_only test

Are these changes tested?

This PR is only a test.

Are there any user-facing changes?

No.

…ing a batch

Add a test documenting that ParquetPushDecoder requests all pages for all
projected columns when it starts a row group, and produces no output until
every requested range is present, even when the pages already pushed are
sufficient to decode the next batch. Also shows that adjacent pushed ranges
are not coalesced to satisfy a larger requested range.

Related to apache#6946

Co-Authored-By: Claude Fable 5.1 <[email protected]>
@github-actions github-actions Bot added the parquet Changes to the parquet crate label Sep 25, 2026
adriangb added a commit to pydantic/arrow-rs that referenced this pull request Sep 26, 2026
* `test_decoder_first_pages_only_batch_granularity`, the counterpart
  of `test_decoder_first_pages_only` from
  apache#11218. That test shows that the
  default `FetchGranularity::RowGroup` does not decode a batch until the
  whole row group is pushed. With `FetchGranularity::Batch`, pushing the
  first page of each column returns the first batch.
* `incremental_tests`: both granularities give the same batches for
  batch sizes around page and row-group sizes, projections with list and
  struct columns, page-skipping row selections with each selection
  policy, offset and limit, files without an offset index, predicate
  chains with and without the predicate cache (including cache misses),
  and 300 random combinations of all of these. Other tests check that
  requests serve one batch, that dictionary pages are requested once, that
  no range is requested twice, that bytes pushed ahead are released as the
  readers pass them, that the row-group boundary is visible after the
  last batch, and `into_builder` and `try_next_reader` interplay.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
adriangb added a commit to pydantic/arrow-rs that referenced this pull request Sep 26, 2026
* `test_decoder_first_page_only_does_not_decode` from
  apache#11218, which shows that the
  row-group mode does not decode a batch until the whole row group is
  pushed, and a counterpart for `FetchGranularity::Batch` where pushing
  the first page of each column returns the first batch.
* `incremental_tests`: both granularities give the same batches for
  batch sizes around page and row-group sizes, projections with list and
  struct columns, page-skipping row selections with each selection
  policy, offset and limit, files without an offset index, predicate
  chains with and without the predicate cache (including cache misses),
  and 300 random combinations of all of these. Other tests check that
  requests serve one batch, that dictionary pages are requested once, that
  no range is requested twice, that bytes pushed ahead are released as the
  readers pass them, that the row-group boundary is visible after the
  last batch, and `into_builder` and `try_next_reader` interplay.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

@adriangb adriangb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here's my token approval, thanks @alamb. Would be great to get this merged.

@alamb
alamb requested a review from etseidl September 28, 2026 20:11
@alamb

alamb commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

@etseidl could I trouble you for a review of this PR (it is a test for a feature @adriangb is working on)

@etseidl

etseidl commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

Sure, I added it to my queue over the weekend, but I'll bump the priority.

@etseidl etseidl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks correct to me.

@alamb

alamb commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

Looks correct to me.

Thank you for the review. I am aware I owe you one too (but I have to run now so I didn't make it today). Will do tomorrow

@alamb
alamb merged commit 21f1156 into apache:main Sep 28, 2026
29 checks passed
@etseidl

etseidl commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

Looks correct to me.

Thank you for the review. I am aware I owe you one too (but I have to run now so I didn't make it today). Will do tomorrow

No worries, I'm in no hurry.

adriangb added a commit to pydantic/arrow-rs that referenced this pull request Sep 28, 2026
* `test_decoder_first_pages_only_batch_granularity`, the counterpart
  of `test_decoder_first_pages_only` from
  apache#11218. That test shows that the
  default `FetchGranularity::RowGroup` does not decode a batch until the
  whole row group is pushed. With `FetchGranularity::Batch`, pushing the
  first page of each column returns the first batch.
* `incremental_tests`: both granularities give the same batches for
  batch sizes around page and row-group sizes, projections with list and
  struct columns, page-skipping row selections with each selection
  policy, offset and limit, files without an offset index, predicate
  chains with and without the predicate cache (including cache misses),
  and 300 random combinations of all of these. Other tests check that
  requests serve one batch, that dictionary pages are requested once, that
  no range is requested twice, that bytes pushed ahead are released as the
  readers pass them, that the row-group boundary is visible after the
  last batch, and `into_builder` and `try_next_reader` interplay.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
adriangb added a commit to pydantic/arrow-rs that referenced this pull request Sep 28, 2026
test_decoder_first_pages_only_batch_granularity is the counterpart of
the test from apache#11218. incremental_tests.rs checks that both
granularities give the same batches (targeted and randomized), and
checks the requests, the row-group boundary and the bytes held.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
adriangb added a commit to pydantic/arrow-rs that referenced this pull request Sep 28, 2026
test_decoder_first_pages_only_batch_granularity is the counterpart of
the test from apache#11218. incremental_tests.rs checks that both
granularities give the same batches (targeted and randomized), and
checks the requests, the row-group boundary and the bytes held.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
adriangb pushed a commit to pydantic/arrow-rs that referenced this pull request Sep 29, 2026
…ing a batch (apache#11218)

# Which issue does this PR close?

- Related to apache#6946

# Rationale for this change

While discussing apache#6946 with
@adriangb, a question came up about whether `ParquetPushDecoder` would
decode a batch if only the pages needed for that batch had been pushed
or if it insists on having all pages for the row group first.

This PR adds a test that answers that question

For the current code, the ParquetPushDecoder requires that all ranges
for the row group are present

# What changes are included in this PR?

 `test_decoder_first_page_only` test 

# Are these changes tested?

This PR is only a test.

# Are there any user-facing changes?

No.

---------

Co-authored-by: Claude Fable 5.1 <[email protected]>
@Jefffrey Jefffrey added the development-process Internal changes; PRs with this label are excluded from changelog label Sep 29, 2026
adriangb added a commit to pydantic/arrow-rs that referenced this pull request Oct 1, 2026
test_decoder_first_pages_only_batch_granularity is the counterpart of
the test from apache#11218. incremental_tests.rs checks that both
granularities give the same batches (targeted and randomized), and
checks the requests, the row-group boundary and the bytes held.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
adriangb added a commit to pydantic/arrow-rs that referenced this pull request Oct 1, 2026
test_decoder_first_pages_only_batch_granularity is the counterpart of
the test from apache#11218. incremental_tests.rs checks that both
granularities give the same batches (targeted and randomized), and
checks the requests, the row-group boundary and the bytes held.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
adriangb added a commit to pydantic/arrow-rs that referenced this pull request Oct 9, 2026
test_decoder_first_pages_only_batch_granularity is the counterpart of
the test from apache#11218. incremental_tests.rs checks that both
granularities give the same batches (targeted and randomized), and
checks the requests, the row-group boundary and the bytes held.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

development-process Internal changes; PRs with this label are excluded from changelog parquet Changes to the parquet crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants