GH-51264: [C++][Parquet] Make MemoryPool settable on ReaderProperties - #51269
Conversation
There was a problem hiding this comment.
🟡 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
…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
|
Also i see anytime someone creates a PR with C++ component 2-3 CI tests fails is that normal or a bug |
Unfortunately a number of our CI jobs are broken currently. |
pitrou
left a comment
There was a problem hiding this comment.
LGTM, thank you @Diveyam-Mishra
|
CI failures are unrelated, I'll merge. |
Let me look into this after this gets merged |
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