-
Notifications
You must be signed in to change notification settings - Fork 4.3k
GH-51264: [C++][Parquet] Make MemoryPool settable on ReaderProperties - #51269
GH-51264: [C++][Parquet] Make MemoryPool settable on ReaderProperties #51269Diveyam-Mishra wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The new unit test can be ineffective on builds where default_memory_pool() equals system_memory_pool(), so it may not actually validate the new setter behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds the ability to mutate parquet::ReaderProperties’ memory pool after construction, enabling arrow::dataset::MakeReaderProperties to copy reader properties wholesale (instead of field-by-field) while still overriding the pool for dataset reads.
Changes:
- Add
ReaderProperties::set_memory_pool(MemoryPool*)to allow post-construction pool updates. - Simplify dataset Parquet
MakeReaderPropertiesby copyingReaderPropertiesand overriding only the pool (and encryption props when configured). - Expose the new API to PyArrow Cython declarations and add a unit test for the new setter.
File summaries
| File | Description |
|---|---|
| python/pyarrow/includes/libparquet.pxd | Exposes memory_pool() and set_memory_pool() for parquet::ReaderProperties to PyArrow. |
| cpp/src/parquet/properties.h | Adds ReaderProperties::set_memory_pool() mutator. |
| cpp/src/parquet/properties_test.cc | Adds a unit test covering set_memory_pool() and copy behavior. |
| cpp/src/arrow/dataset/file_parquet.cc | Simplifies MakeReaderProperties by copying properties and then setting the pool (and encryption props when applicable). |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9fa734e to
576e470
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟢 Approval recommended
Small, well-scoped API addition with corresponding unit test and a straightforward Dataset integration simplification.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to unset read_dense_for_nullable as it wasn't set before?
What do you think @wgtmac ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense if we want to strictly keep the previous behavior. That said, I don't expect users to set read_dense_for_nullable to true when using the dataset reader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed the change so it remains false
...erties and simplify dataset MakeReaderProperties Rationale for this change In arrow::dataset::MakeReaderProperties, all parquet::ReaderProperties options had to be copied field-by-field from parquet_scan_options->reader_properties because ReaderProperties did not support mutating or setting MemoryPool* after construction. What changes are included in this PR? Add set_memory_pool to parquet::ReaderProperties in cpp/src/parquet/properties.h. Add unit test TestReaderProperties.SetMemoryPool in cpp/src/parquet/properties_test.cc. Are these changes tested? Yes Are there any user-facing changes? No
576e470 to
3306174
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟢 Approval recommended
No unresolved review comments were identified, and all assessments indicate approval readiness.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
Diveyam-Mishra
commented
Sep 10, 2026
Also i see anytime someone creates a PR with C++ component 2-3 CI tests fails is that normal or a bug
Uh oh!
There was an error while loading. Please reload this page.
Rationale for this change
In arrow::dataset::MakeReaderProperties, all parquet::ReaderProperties options had to be copied field-by-field from parquet_scan_options->reader_properties because ReaderProperties did not support mutating or setting MemoryPool* after construction.
What changes are included in this PR?
Added set_memory_pool to parquet::ReaderProperties in cpp/src/parquet/properties.h.
Added unit test TestReaderProperties.SetMemoryPool in cpp/src/parquet/properties_test.cc.
Are these changes tested?
Yes
Are there any user-facing changes?
No