Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: Add additional info about memory reservation to the doc of MemoryPool #14789

Merged
merged 1 commit into from
Feb 20, 2025

Conversation

Kontinuation
Copy link
Member

Which issue does this PR close?

This is not related to an issue. It is requested by this comment: #14644 (comment)

Rationale for this change

There's a convention that ExecutionPlans do not reserve memory for the batches it produces, but it is not explicitly stated in the doc. We should explicitly point out those conventions in the doc to make it clear.

What changes are included in this PR?

This change clarifies the general conventions that ExecutionPlans need to follow when using MemoryPool: memory for output batches are reserved by their consumers rather than their producers.

Are these changes tested?

N/A for doc changes.

Are there any user-facing changes?

No

@github-actions github-actions bot added the execution Related to the execution crate label Feb 20, 2025
@Kontinuation Kontinuation changed the title minor: Add additional info about memory reservation to the doc of MemoryPool docs: Add additional info about memory reservation to the doc of MemoryPool Feb 20, 2025
@Kontinuation Kontinuation marked this pull request as ready for review February 20, 2025 09:59
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @Kontinuation

I think this is a very nice and clear addition

@alamb alamb added the documentation Improvements or additions to documentation label Feb 20, 2025
Copy link
Contributor

@zhuqi-lucas zhuqi-lucas left a comment

Choose a reason for hiding this comment

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

LGTM Thanks!

@xudong963 xudong963 merged commit 84232d8 into apache:main Feb 20, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation execution Related to the execution crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants