You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "jonathanc-n (via GitHub)" <gi...@apache.org> on 2025/09/05 02:51:29 UTC
[PR] refactor: Use `BufferedBatchState` enum for SMJ spilling [datafusion]
jonathanc-n opened a new pull request, #17429:
URL: https://github.com/apache/datafusion/pull/17429
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
This was brought up by @alamb in the SMJ spilling pull request.
## Rationale for this change
Using enum to represent batch spill state is simpler than two enums
Wanted to get this out of the way before switching to SMJ during runtime.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
Use enum instead of double option for expressing the state of the buffered batch
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Use `BufferedBatchState` enum for SMJ spilling [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on PR #17429:
URL: https://github.com/apache/datafusion/pull/17429#issuecomment-3256909358
cc @alamb @comphead
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Use `BufferedBatchState` enum for SMJ spilling [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead merged PR #17429:
URL: https://github.com/apache/datafusion/pull/17429
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Use `BufferedBatchState` enum for SMJ spilling [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #17429:
URL: https://github.com/apache/datafusion/pull/17429#discussion_r2323986334
##########
datafusion/physical-plan/src/joins/sort_merge_join/stream.rs:
##########
@@ -849,11 +854,13 @@ impl SortMergeJoinStream {
fn free_reservation(&mut self, buffered_batch: BufferedBatch) -> Result<()> {
// Shrink memory usage for in-memory batches only
- if buffered_batch.spill_file.is_none() && buffered_batch.batch.is_some() {
- self.reservation
- .try_shrink(buffered_batch.size_estimation)?;
+ match buffered_batch.batch {
Review Comment:
This is done like so to avoid having to add `PartialEq` for `RefCountedTempFile`
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Use `BufferedBatchState` enum for SMJ spilling [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #17429:
URL: https://github.com/apache/datafusion/pull/17429#discussion_r2324056495
##########
datafusion/physical-plan/src/joins/sort_merge_join/stream.rs:
##########
@@ -185,11 +185,12 @@ impl StreamedBatch {
}
/// A buffered batch that contains contiguous rows with same join key
+///
+/// `BufferedBatch` can exist as either an in-memory `RecordBatch` or a `RefCountedTempFile` on disk.
#[derive(Debug)]
pub(super) struct BufferedBatch {
- /// The buffered record batch
- /// None if the batch spilled to disk th
- pub batch: Option<RecordBatch>,
+ /// Represents in memory or spilled record batch
+ pub batch: BufferedBatchState,
Review Comment:
I think this `BufferedBatchState` enum should include all intermediate data into the states, for example `join_arrays`. When the main batch is spilled, the join key array should either be also spilled or throw away, otherwise it will be leaking memory.
Perhaps we can leave a TODO in the comment now? This will be a big change.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Use `BufferedBatchState` enum for SMJ spilling [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #17429:
URL: https://github.com/apache/datafusion/pull/17429#discussion_r2323986334
##########
datafusion/physical-plan/src/joins/sort_merge_join/stream.rs:
##########
@@ -849,11 +854,13 @@ impl SortMergeJoinStream {
fn free_reservation(&mut self, buffered_batch: BufferedBatch) -> Result<()> {
// Shrink memory usage for in-memory batches only
- if buffered_batch.spill_file.is_none() && buffered_batch.batch.is_some() {
- self.reservation
- .try_shrink(buffered_batch.size_estimation)?;
+ match buffered_batch.batch {
Review Comment:
This is done like so to avoid having to add `PartialEq` for `RefCountedTempFile`
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org