Skip to content

GH-54637: [Python] add MemoryPool support to pq.read_table - #54919

Open
omkar-334 wants to merge 2 commits into
apache:mainfrom
omkar-334:GH-54637-parquet-read-table-memory-pool
Open

omkar-334 wants to merge 2 commits into
apache:mainfrom
omkar-334:GH-54637-parquet-read-table-memory-pool

Conversation

@omkar-334

@omkar-334 omkar-334 commented Oct 21, 2026 •

Copy link
Copy Markdown

Rationale for this change

pq.read_table has no way to pass a memory pool. ParquetReader and Dataset.to_table already accept one, it just wasn't added.

What changes are included in this PR?

I added a memory_pool argument to read_table, ParquetFile and ParquetDataset. It is passed to ParquetReader and to Dataset.to_table, with docstrings updated.

Are these changes tested?

Yes. I added new tests in test_basic.py - It reads a Parquet file with a tracked memory pool and check that the pool was actually used. They cover read_table, ParquetFile and ParquetDataset.

Are there any user-facing changes?

New optional memory_pool keyword. No breaking changes.

Was AI used for this PR?

PR code and description written by:

  • Human
  • AI

Reviewed before submission by:

  • Human
  • AI

Github Issue - #54637

Copilot AI balanced review requested due to automatic review settings October 21, 2026 22:01

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added the awaiting review Awaiting review label Oct 21, 2026
@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #54637 has been automatically assigned in GitHub to PR creator.

@omkar-334

omkar-334 commented Oct 21, 2026 •

Copy link
Copy Markdown
Author

My understanding before implementing/opening the PR -

Function read_table supports 2 types of datasets

  1. dataset = ParquetDataset
  • self._dataset = ds.dataset
    This function returns multiple types, not so straightforward. The class Dataset (_dataset.pyx) has three types of datasets -

    classes = {
          'union': UnionDataset,
          'filesystem': FileSystemDataset,
          'in-memory': InMemoryDataset,
      }
    
    • _filesystem_dataset -> FileSystemDatasetFactory
    • _union_dataset -> UnionDataset
    • _in_memory_dataset -> InMemoryDataset
      We don;t need to change code for all the classes, the below unction makes it easy for us
  • table = self._dataset.to_table -> to_table (_dataset.pyx)
    This function in pyx supports MemoryPool -> we just need to edit this.

  1. dataset = ParquetFile
  • self.reader = ParquetReader()
    • python/pyarrow/_parquet.pyx - ParquetReader supports CMemoryPool* pool
  • self.reader.read_all
  • return dataset.read

@omkar-334

Copy link
Copy Markdown
Author
image Tests passing

@omkar-334

Copy link
Copy Markdown
Author

cc @pitrou

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for doing this @omkar-334 . This looks good on the principle, just a small suggestion for improving tests.


result = pq.read_table(path, memory_pool=proxy_pool)
assert result.equals(table)
assert proxy_pool.max_memory() > 0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about we exercise this a bit more specifically, such as:

Suggested change
assert proxy_pool.max_memory() > 0
assert proxy_pool.bytes_allocated() >= table.nbytes
result = None
assert proxy_pool.bytes_allocated() == 0

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants