You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "joe-ucp (via GitHub)" <gi...@apache.org> on 2025/11/17 19:30:51 UTC
[PR] Change default behavior for Parquet PageEncodingStats #8859 [arrow-rs]
joe-ucp opened a new pull request, #8860:
URL: https://github.com/apache/arrow-rs/pull/8860
- Decode page encoding stats to a compact `EncodingMask` by default
- Add `PageEncodingStatsMode { Mask, Full, Skip }` and `ParquetPageEncodingStats`
- Keep on-disk Parquet metadata format unchanged
- Update tests, benchmarks and docs for the new default
# Which issue does this PR close?
- Closes #8859.
# Rationale for this change
Decoding page encoding statistics into a full `Vec<PageEncodingStats>` for every column can allocate a lot of memory and is rarely needed by callers. A compact mask representation keeps enough information for most use cases (which encodings appear) while reducing allocations and metadata parsing cost.
At the same time, we still want to support users who rely on the full stats, and also allow completely skipping stats when they are not needed.
# What changes are included in this PR?
- Introduce `PageEncodingStatsMode` with three modes:
- `Mask` (default): decode stats to a compact `EncodingMask`
- `Full`: decode full `Vec<PageEncodingStats>` as before
- `Skip`: do not decode page encoding stats
- Introduce `ParquetPageEncodingStats` to store stats in memory as either:
- `Mask(EncodingMask)` or
- `Full(Vec<PageEncodingStats>)`
- Extend `ParquetMetaDataOptions` with `page_encoding_stats_mode`, defaulting to `Mask`
- Wire `PageEncodingStatsMode` into the Thrift metadata decoder to:
- derive a mask from the decoded stats,
- preserve the full vector, or
- skip decoding entirely, depending on the mode
- Keep serialization/on-disk format unchanged (only full stats are written when present)
- Update `ColumnChunkMetaData` to use `ParquetPageEncodingStats` and add helpers:
- `page_encoding_stats()`
- `page_encoding_stats_mask()`
- `page_encoding_stats_full()`
- Update and extend tests and benchmarks for all three modes
- Update documentation and `CHANGELOG.md` to describe the new default and how to opt into full stats
# Are these changes tested?
Yes:
- `cargo fmt -p parquet`
- `cargo clippy -p parquet -- -D warnings`
- `cargo test -p parquet`
- including new tests for Mask / Full / Skip modes and edge cases
- `cargo bench -p parquet -- benches::metadata`
- Mask mode performs at least as well as the previous default, with Full and Skip behaving as expected
# Are there any user-facing changes?
Yes:
- The default behavior when decoding metadata now uses `Mask` mode instead of always decoding full `Vec<PageEncodingStats>`.
- Users who need the full stats can opt in via:
- `ParquetMetaDataOptions::with_page_encoding_stats_mode(PageEncodingStatsMode::Full)`
- Users who do not need page encoding stats can select `PageEncodingStatsMode::Skip`.
This is an in-memory behavior change intended for the next **major release**; the Parquet on-disk format is unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change default behavior for Parquet PageEncodingStats #8859 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8860:
URL: https://github.com/apache/arrow-rs/pull/8860#issuecomment-3543997995
Thanks for your enthusiasm 😄 You implemented in an hour what it took me several weeks to arrive at 😅. Please take a look at our many open issues and see if there are any others that pique your interest. Given what you've submitted here I don't think you need to limit yourself to good first issues. Thanks again!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change default behavior for Parquet PageEncodingStats #8859 [arrow-rs]
Posted by "joe-ucp (via GitHub)" <gi...@apache.org>.
joe-ucp commented on PR #8860:
URL: https://github.com/apache/arrow-rs/pull/8860#issuecomment-3543950321
Thanks for the clarification, that makes sense! 😭
I misunderstood the scope of #8859 and thought it needed more structural changes on top of what was already in main. If #8797 already implements most of this, I’m happy to close this PR and (once #8797 is merged) open a small follow-up that just flips the default for `encoding_stats_as_mask` to `true` and updates any docs/changelog as needed.
Appreciate the quick feedback 🙏
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change default behavior for Parquet PageEncodingStats #8859 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8860:
URL: https://github.com/apache/arrow-rs/pull/8860#discussion_r2535645835
##########
CHANGELOG.md:
##########
@@ -29,6 +29,7 @@
- feat: Improve DataType display for `RunEndEncoded` [\#8596](https://github.com/apache/arrow-rs/pull/8596) [[arrow](https://github.com/apache/arrow-rs/labels/arrow)] ([Weijun-H](https://github.com/Weijun-H))
- Add `ArrowError::AvroError`, remaining types and roundtrip tests to `arrow-avro`, [\#8595](https://github.com/apache/arrow-rs/pull/8595) [[arrow](https://github.com/apache/arrow-rs/labels/arrow)] ([jecsand838](https://github.com/jecsand838))
- \[thrift-remodel\] Refactor Thrift encryption and store encodings as bitmask [\#8587](https://github.com/apache/arrow-rs/pull/8587) [[parquet](https://github.com/apache/arrow-rs/labels/parquet)] ([etseidl](https://github.com/etseidl))
+- Change default page encoding stats decoding mode from `Full` to `Mask` in `ParquetMetaDataOptions` [\#8859](https://github.com/apache/arrow-rs/pull/8859) [[parquet](https://github.com/apache/arrow-rs/labels/parquet)] ([user](https://github.com/user))
Review Comment:
PRs shouldn't be changing the changelog for an already released version
@joe-ucp I suspect this PR is created by claude/codex or a similar tool. This is totally fine but we need your help to review the PR first before asking us to review it
It is not yet an arrow project guideline, but I personally think the guidelines from https://datafusion.apache.org/contributor-guide/index.html#ai-assisted-contributions helps undertand the background and what we generally expect from contrubutors
Thanks again
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change default behavior for Parquet PageEncodingStats #8859 [arrow-rs]
Posted by "joe-ucp (via GitHub)" <gi...@apache.org>.
joe-ucp commented on code in PR #8860:
URL: https://github.com/apache/arrow-rs/pull/8860#discussion_r2535699733
##########
CHANGELOG.md:
##########
@@ -29,6 +29,7 @@
- feat: Improve DataType display for `RunEndEncoded` [\#8596](https://github.com/apache/arrow-rs/pull/8596) [[arrow](https://github.com/apache/arrow-rs/labels/arrow)] ([Weijun-H](https://github.com/Weijun-H))
- Add `ArrowError::AvroError`, remaining types and roundtrip tests to `arrow-avro`, [\#8595](https://github.com/apache/arrow-rs/pull/8595) [[arrow](https://github.com/apache/arrow-rs/labels/arrow)] ([jecsand838](https://github.com/jecsand838))
- \[thrift-remodel\] Refactor Thrift encryption and store encodings as bitmask [\#8587](https://github.com/apache/arrow-rs/pull/8587) [[parquet](https://github.com/apache/arrow-rs/labels/parquet)] ([etseidl](https://github.com/etseidl))
+- Change default page encoding stats decoding mode from `Full` to `Mask` in `ParquetMetaDataOptions` [\#8859](https://github.com/apache/arrow-rs/pull/8859) [[parquet](https://github.com/apache/arrow-rs/labels/parquet)] ([user](https://github.com/user))
Review Comment:
@alamb Thanks for calling this out, and for the link to the AI-assisted contribution guidelines.
You’re right: I used an LLM/Copilot-style workflow to help draft this change. I did run `cargo fmt`, `cargo clippy`, and `cargo test -p parquet`, but I’m still pretty new to Git/GitHub flow and self-taught, so I missed that I shouldn’t touch the changelog for already released versions and that I was overlaping with the work in #8797.
For this PR I’m happy to let it be closed in favor of #8797.
I’ll make sure future contributions are better self-reviewed before I open a PR and that I follow the AI guidelines you linked. Thanks again for the feedback (and the patience)!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change default behavior for Parquet PageEncodingStats #8859 [arrow-rs]
Posted by "joe-ucp (via GitHub)" <gi...@apache.org>.
joe-ucp closed pull request #8860: Change default behavior for Parquet PageEncodingStats #8859
URL: https://github.com/apache/arrow-rs/pull/8860
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Change default behavior for Parquet PageEncodingStats #8859 [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #8860:
URL: https://github.com/apache/arrow-rs/pull/8860#issuecomment-3543743808
Thanks for you submission @joe-ucp! This is a bit early since #8797 hasn't yet merged and is still in flux. I think many of the changes here are already implemented in that PR. The issue was simply to change the default for `encoding_stats_as_mask` to `true` by default 😅.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org