Re: [PR] Cascaded spill merge and re-spill [datafusion]
2010YOUY01 closed pull request #15610: Cascaded spill merge and re-spill URL: https://github.com/apache/datafusion/pull/15610 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
2010YOUY01 commented on PR #15610:
URL: https://github.com/apache/datafusion/pull/15610#issuecomment-3034782742
> > tested my fuzz tests with this pr and all of them are failing currently
>
> Update: I think the failure is not due to this PR's implementation,
instead it's caused by `FairMemoryPool`'s limitation.
>
> After manually setting `max_spill_merge_degree` to 2, first 3 tests
passed, the 4th one failed with
>
> ```
> Error: ResourcesExhausted("Additional allocation failed with top memory
consumers (across reservations) as: mock_memory_consumer#2(can spill: false)
consumed 1695480 bytes, ExternalSorterMerge[0]#1(can spill: false) consumed
401664 bytes. Error: Failed to allocate additional 297024 bytes for
ExternalSorterMerge[0] with 0 bytes already allocated for this reservation - 8
bytes remain available for the total pool
> ```
>
> I believe it's the limitation of `FairSpillPool` that non-spillable
consumers are not able to back off, and it can block spilling consumers from
normal execution. (and this specific test is also possible to pass due to some
complex interactions if the runtime memory consumers are set up differently)
>
> I'll try to come up with a minimal reproducer later.
I spotted a critical issue in this PR:
This approach works if the memory pool is only used by a single query, since
we can guarantee during different stage in sort operator (partial sort -> SPM),
the memory limit for a partition is roughly the same.
However, if a memory pool is shared among many queries (new query join the
pool, get finished, and leave): given a certain partition, its memory budget
can vary in its life cycle. For example: for a sort operator, when doing
partial sorting it has 2GB memory budget, but when it moves to SPM phase, it
only got 1GB because a new memory-consuming neighbor has recently joined.
As a result, SPM has to be inherently spillable.
I plan to close this PR and help to get
https://github.com/apache/datafusion/pull/15700 merged, I have a rough idea to
do a patch for my previous concern, I'll share my ideas there.
cc @rluvaton @ding-young
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
2010YOUY01 commented on PR #15610: URL: https://github.com/apache/datafusion/pull/15610#issuecomment-3034531555 > This PR has a significant strength in that it works reliably even under a fairly conservative memory limit, which is impressive. I also learned a lot while reviewing it :). However, I do have the following concern about the approach based on user-configured `MAX_SPILL_MERGE_DEGREE`: > > I don’t think adding a user-configurable merge degree is inherently a bad idea. The problem is that the appropriate value for `MAX_SPILL_MERGE_DEGREE` can vary significantly depending on the query being executed and the characteristics of the spilled `RecordBatches`. For example, merging many thin batches is very different from merging a few wide batches in terms of memory consumption, and the optimal degree will differ accordingly. > > Unless the degree is defined in a way that consistently reflects some notion of aggressiveness or is based on actual memory consumption (e.g., in bytes), it would be hard to offer meaningful tuning guidance to users. In the end, as @rluvaton suggested, I think we do need to estimate something like `max_memory_bytes` per batch—even if the estimate isn’t very reliable. > > Though relying on memory estimation may not be very reliable, it seems unavoidable in the context of multi-pass merge to estimate the memory size of the RecordBatches to be read from each spill file. This is because we need at least a rough estimate to decide how many spill files (or streams) can be read and merged simultaneously, so that we could automatically perform multi-pass merge without user's manual debugging. > > What do you think about this? 🤔 I agree this knob would be very hard to be tuned manually. I was thinking the next step to be adding an `auto` option to the config, and automatically determine this max merge degree based on stats, according to avg batch size and memory budget. If it's wrong, the memory pool reports OOM, maybe the executor can retry with a small constant merge degree as the fallback path. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
rluvaton commented on PR #15610: URL: https://github.com/apache/datafusion/pull/15610#issuecomment-3031737782 > Of course, I’m also currently trying to reproduce the case you pointed out in #15700, so if I discover a significant issue there, my opinion may very well change. @ding-young I already created a pr that reproduced the issues I outlined: - #15727 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
ding-young commented on PR #15610: URL: https://github.com/apache/datafusion/pull/15610#issuecomment-3031719770 This PR has a significant strength in that it works reliably even under a fairly conservative memory limit, which is impressive. I also learned a lot while reviewing it :). However, I do have the following concern about the approach based on user-configured `MAX_SPILL_MERGE_DEGREE`: I don’t think adding a user-configurable merge degree is inherently a bad idea. The problem is that the appropriate value for `MAX_SPILL_MERGE_DEGREE` can vary significantly depending on the query being executed and the characteristics of the spilled `RecordBatches`. For example, merging many thin batches is very different from merging a few wide batches in terms of memory consumption, and the optimal degree will differ accordingly. Unless the degree is defined in a way that consistently reflects some notion of aggressiveness or is based on actual memory consumption (e.g., in bytes), it would be hard to offer meaningful tuning guidance to users. In the end, as @rluvaton suggested, I think we do need to estimate something like `max_memory_bytes` per batch—even if the estimate isn’t very reliable. Though relying on memory estimation may not be very reliable, it seems unavoidable in the context of multi-pass merge to estimate the memory size of the RecordBatches to be read from each spill file. This is because we need at least a rough estimate to decide how many spill files (or streams) can be read and merged simultaneously, so that we could automatically perform multi-pass merge without user's manual debugging. What do you think about this? :thinking: Of course, I’m also currently trying to reproduce the case you pointed out in #15700, so if I discover a significant issue there, my opinion may very well 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
alamb commented on PR #15610:
URL: https://github.com/apache/datafusion/pull/15610#issuecomment-2869674992
> I've tried to use this branch to sort data larger than memory. For 24GB
parquet file, it produce error `Error: ArrowError(IoError("No space left on
device (os error 28)", Os { code: 28, kind: StorageFull, message: "No space
left on device" }), None)` (i think this suggest it uses up all 100GB default
DiskManager?)
We probably need to enable compression for such datasets (parquet is quite a
bit more compressed than Arrow)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
LogicFan commented on PR #15610:
URL: https://github.com/apache/datafusion/pull/15610#issuecomment-2869669114
I've tried to use this branch to sort data larger than memory. For 24GB
parquet file, it produce error `Error: ArrowError(IoError("No space left on
device (os error 28)", Os { code: 28, kind: StorageFull, message: "No space
left on device" }), None)` (i think this suggest it uses up all 100GB default
DiskManager?)
here is the configuration i use
```
let cfg = SessionConfig::new()
.with_sort_max_spill_merge_degree(2)
.with_sort_spill_reservation_bytes(1 << 10)
.with_sort_in_place_threshold_bytes(1 << 10)
.set_usize("datafusion.execution.batch_size", 16)
.set_usize("datafusion.execution.soft_max_rows_per_output_file",
2048)
.set_usize("datafusion.execution.minimum_parallel_output_files", 32);
let memory_pool = Arc::new(TrackConsumersPool::new(
FairSpillPool::new(16 * (1 << 30)),
NonZeroUsize::new(5).unwrap(),
));
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
2010YOUY01 commented on PR #15610:
URL: https://github.com/apache/datafusion/pull/15610#issuecomment-2812536781
> tested my fuzz tests with this pr and all of them are failing currently
Update: I think the failure is not due to this PR's implementation, instead
it's caused by `FairMemoryPool`'s limitation.
After manually setting `max_spill_merge_degree` to 2, first 3 tests passed,
the 4th one failed with
```
Error: ResourcesExhausted("Additional allocation failed with top memory
consumers (across reservations) as: mock_memory_consumer#2(can spill: false)
consumed 1695480 bytes, ExternalSorterMerge[0]#1(can spill: false) consumed
401664 bytes. Error: Failed to allocate additional 297024 bytes for
ExternalSorterMerge[0] with 0 bytes already allocated for this reservation - 8
bytes remain available for the total pool
```
I believe it's the limitation of `FairSpillPool` that non-spillable
consumers are not able to back off, and it can block spilling consumers from
normal execution. (and this specific test is also possible to pass due to some
complex interactions if the runtime memory consumers are set up differently)
I'll try to come up with a minimal reproducer later.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
rluvaton commented on PR #15610: URL: https://github.com/apache/datafusion/pull/15610#issuecomment-2812202064 In the PR that I created for fuzz tests that are also tests on sort which fails here as well - https://github.com/apache/datafusion/pull/15727 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
2010YOUY01 commented on PR #15610: URL: https://github.com/apache/datafusion/pull/15610#issuecomment-2811652139 > Thank you, can you please take the fuzz test that I created in my pr and add it to yours, making sure it will pass (it will require you updating `row_hash.rs` file Those tests are great, but I think it's outside the scope of this PR: now external aggregation is using a different path for handling spills, the failures are not the regression caused by this PR. It makes more sense to me to do it as a follow-up to: 1. Reuse spill handling code inside external aggr 2. make sure those tests pass. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
2010YOUY01 commented on PR #15610: URL: https://github.com/apache/datafusion/pull/15610#issuecomment-2808240189 > > Benchmark results: (I think there is no significant regression for an extra round of re-spill, if it's running on a machine with fast SSDs) > > It seems to me that there is a 30% regression in performance compared to main when there is enough memory, right? > > > Result > > Main (1.2G): > > Q7 avg time: 8680.47 ms > > PR (1.2G): > > Q7 avg time: 11808.71 ms > > But this PR is significantly better that it can complete with only 500M of memory > > Is there any way to regain the performance (maybe by choosing how many merge phases to do based on available memory rather than a fixed size)? If we manually set this max merge degree to a larger value, the merging behavior will be equivalent to the current implementation: ``` Q7 iteration 0 took 7242.8 ms and returned 59986052 rows Q7 iteration 1 took 7203.4 ms and returned 59986052 rows Q7 iteration 2 took 9812.6 ms and returned 59986052 rows Q7 avg time: 8086.24 ms ``` I think auto-tuning is possible, and is also a good future optimization to do, but it requires some work to extend the memory pool to estimate available memory for current reservation. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
rluvaton commented on code in PR #15610:
URL: https://github.com/apache/datafusion/pull/15610#discussion_r2045620030
##
datafusion/common/src/config.rs:
##
@@ -337,6 +337,13 @@ config_namespace! {
/// batches and merged.
pub sort_in_place_threshold_bytes: usize, default = 1024 * 1024
+/// When doing external sorting, the maximum number of spilled files to
+/// read back at once. Those read files in the same merge step will be
sort-
+/// preserving-merged and re-spilled, and the step will be repeated to
reduce
+/// the number of spilled files in multiple passes, until a final
sorted run
+/// can be produced.
+pub sort_max_spill_merge_degree: usize, default = 16
Review Comment:
I'm keeping open my alternative approach as it seems working in limited
memory envs as well (tested it locally with more data)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
rluvaton commented on PR #15610: URL: https://github.com/apache/datafusion/pull/15610#issuecomment-2807586373 tested my fuzz tests with this pr and al of them are failing currently -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
rluvaton commented on PR #15610: URL: https://github.com/apache/datafusion/pull/15610#issuecomment-2807435301 > > Thank you, can you please take the fuzz test that I created in my pr and add it to yours, making sure it will pass (it will require you updating row_hash.rs file > > @rluvaton is there any way to make a PR with only the fuzz test in it (perhaps with some comments on what would pass/fail once we have this multi-pass algorithm @alamb created https://github.com/apache/datafusion/pull/15727 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
alamb commented on PR #15610: URL: https://github.com/apache/datafusion/pull/15610#issuecomment-2807165302 > Thank you, can you please take the fuzz test that I created in my pr and add it to yours, making sure it will pass (it will require you updating row_hash.rs file @rluvaton is there any way to make a PR with only the fuzz test in it (perhaps with some comments on what would pass/fail once we have this multi-pass algorithm -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
alamb commented on PR #15610: URL: https://github.com/apache/datafusion/pull/15610#issuecomment-2807165186 > Benchmark results: (I think there is no significant regression for an extra round of re-spill, if it's running on a machine with fast SSDs) It seems to me that there is a 30% regression in performance compared to main when there is enough memory, right? > Result > Main (1.2G): > Q7 avg time: 8680.47 ms > > PR (1.2G): > Q7 avg time: 11808.71 ms But this PR is significantly better that it can complete with only 500M of memory Is there any way to regain the performance (maybe by choosing how many merge phases to do based on available memory rather than a fixed size)? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
rluvaton commented on PR #15610: URL: https://github.com/apache/datafusion/pull/15610#issuecomment-2800868604 Thank you, can you please take the fuzz test that I created in my pr and add it to yours, making sure it will pass (it will require you updating `row_hash.rs` file -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
2010YOUY01 commented on PR #15610: URL: https://github.com/apache/datafusion/pull/15610#issuecomment-2800408050 > > > Also, to have a fully working larger than memory sort, you need to spill in > > > https://github.com/apache/datafusion/blob/362fcdfc7b9e00cb6126a0cbc41c9abb2637c563/datafusion/physical-plan/src/sorts/builder.rs#L74 > > > > > > In case the memory reservation is failing > > > > > > Could you elaborate? I don't get it. > > Maybe the description for #15700 might help Thank you for providing an alternative approach. I described my primary concern in https://github.com/apache/datafusion/pull/15700#discussion_r2041372025, I think it is not realistic to determine a batch’s memory size after a spilling roundtrip due to the implementation complexity. In such cases, if the estimation is off by a factor of 2, the actual memory usage could also increase by a factor of 2, which is not ideal. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
2010YOUY01 commented on code in PR #15610:
URL: https://github.com/apache/datafusion/pull/15610#discussion_r2041368520
##
datafusion/common/src/config.rs:
##
@@ -337,6 +337,13 @@ config_namespace! {
/// batches and merged.
pub sort_in_place_threshold_bytes: usize, default = 1024 * 1024
+/// When doing external sorting, the maximum number of spilled files to
+/// read back at once. Those read files in the same merge step will be
sort-
+/// preserving-merged and re-spilled, and the step will be repeated to
reduce
+/// the number of spilled files in multiple passes, until a final
sorted run
+/// can be produced.
+pub sort_max_spill_merge_degree: usize, default = 16
Review Comment:
> The reason why I'm picky about this is that it is a new configuration that
will be hard to deprecate or change
This is a solid point, this option is intended to be manually set, and it
has to ensure `(max_batch_size * per_partition_merge_degree * partition_count)
< total_memory_limit`. If it's set correctly for a query, then the query should
succeed.
The problem is the ever-growing number of configurations in DataFusion, and
it seems impossible to set them all correctly. Enabling parallel merging
optimization would require introducing yet another configuration, I'm also
trying to avoid that (though too-many-configs problem might be a harsh reality
we must accept).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
rluvaton commented on PR #15610: URL: https://github.com/apache/datafusion/pull/15610#issuecomment-2800097814 > > Also, to have a fully working larger than memory sort, you need to spill in > > https://github.com/apache/datafusion/blob/362fcdfc7b9e00cb6126a0cbc41c9abb2637c563/datafusion/physical-plan/src/sorts/builder.rs#L74 > > > > In case the memory reservation is failing > > Could you elaborate? I don't get it. Look at the description for https://github.com/apache/datafusion/pull/15700 and the code for the PR it might clarify better -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
rluvaton commented on code in PR #15610:
URL: https://github.com/apache/datafusion/pull/15610#discussion_r2041196922
##
datafusion/common/src/config.rs:
##
@@ -337,6 +337,13 @@ config_namespace! {
/// batches and merged.
pub sort_in_place_threshold_bytes: usize, default = 1024 * 1024
+/// When doing external sorting, the maximum number of spilled files to
+/// read back at once. Those read files in the same merge step will be
sort-
+/// preserving-merged and re-spilled, and the step will be repeated to
reduce
+/// the number of spilled files in multiple passes, until a final
sorted run
+/// can be produced.
+pub sort_max_spill_merge_degree: usize, default = 16
Review Comment:
@2010YOUY01 and @alamb I hope before you merge this PR to look at
https://github.com/apache/datafusion/pull/15700 to see what I mean
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
rluvaton commented on code in PR #15610:
URL: https://github.com/apache/datafusion/pull/15610#discussion_r2041170126
##
datafusion/common/src/config.rs:
##
@@ -337,6 +337,13 @@ config_namespace! {
/// batches and merged.
pub sort_in_place_threshold_bytes: usize, default = 1024 * 1024
+/// When doing external sorting, the maximum number of spilled files to
+/// read back at once. Those read files in the same merge step will be
sort-
+/// preserving-merged and re-spilled, and the step will be repeated to
reduce
+/// the number of spilled files in multiple passes, until a final
sorted run
+/// can be produced.
+pub sort_max_spill_merge_degree: usize, default = 16
Review Comment:
I have a concern about this that there can still be memory issue, if the
batch from each stream together is above the memory limit
I have an implementation for this that is completely memory safe and will
try to create a PR for that for inspiration
The way to decide on the degree is actually by storing for each spill file
the largest amount of memory a single record batch taken, and then when
deciding on the degree, you simply grow until you can no longer.
The reason why I'm picky about this is that it is a new configuration that
will be hard to deprecate or 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
rluvaton commented on code in PR #15610:
URL: https://github.com/apache/datafusion/pull/15610#discussion_r2041170126
##
datafusion/common/src/config.rs:
##
@@ -337,6 +337,13 @@ config_namespace! {
/// batches and merged.
pub sort_in_place_threshold_bytes: usize, default = 1024 * 1024
+/// When doing external sorting, the maximum number of spilled files to
+/// read back at once. Those read files in the same merge step will be
sort-
+/// preserving-merged and re-spilled, and the step will be repeated to
reduce
+/// the number of spilled files in multiple passes, until a final
sorted run
+/// can be produced.
+pub sort_max_spill_merge_degree: usize, default = 16
Review Comment:
I have a concern about this that there can still be memory issue, if the
batch from each stream together is above the memory limit
I have an implementation for this that is completely memory safe and will
try to create a PR for that for inspiration
The way to decide on the degree is actually by storing for each spill file
the largest amount of memory a single record batch taken, and then when
deciding on the degree, you simply grow until you can no longer.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
alamb commented on PR #15610: URL: https://github.com/apache/datafusion/pull/15610#issuecomment-2799903999 I plan to re-review this tomorrow -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
qstommyshu commented on PR #15610: URL: https://github.com/apache/datafusion/pull/15610#issuecomment-2799447585 > ### Intended optimization > If the memory pool is enough to hold more batches at a time (while `spill_max_spill_merge_degree` is still limited to 4, in case the merge-degree is too large and hurt performance in some cases) One additional config `sort_buffer_batch_capacity` is introduced, and set to `16` in the above example, the execution will look like: ... Thanks for the clear explanation, that's a lot of great works, and looks really cool! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
qstommyshu commented on code in PR #15610: URL: https://github.com/apache/datafusion/pull/15610#discussion_r2040846556 ## docs/source/user-guide/configs.md: ## @@ -84,6 +84,7 @@ Environment variables are read during `SessionConfig` initialisation so they mus | datafusion.execution.skip_physical_aggregate_schema_check | false | When set to true, skips verifying that the schema produced by planning the input of `LogicalPlan::Aggregate` exactly matches the schema of the input plan. When set to false, if the schema does not match exactly (including nullability and metadata), a planning error will be raised. This is used to workaround bugs in the planner that are now caught by the new schema verification step. | | datafusion.execution.sort_spill_reservation_bytes | 10485760 | Specifies the reserved memory for each spillable sort operation to facilitate an in-memory merge. When a sort operation spills to disk, the in-memory data must be sorted and merged before being written to a file. This setting reserves a specific amount of memory for that in-memory sort/merge process. Note: This setting is irrelevant if the sort operation cannot spill (i.e., if there's no `DiskManager` configured). | | datafusion.execution.sort_in_place_threshold_bytes | 1048576 | When sorting, below what size should data be concatenated and sorted in a single RecordBatch rather than sorted in batches and merged. | +| datafusion.execution.sort_max_spill_merge_degree| 16 | When doing external sorting, the maximum number of spilled files to read back at once. Those read files in the same merge step will be sort- preserving-merged and re-spilled, and the step will be repeated to reduce the number of spilled files in multiple passes, until a final sorted run can be produced. | Review Comment: Great attention to detail, for updating the user guide! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Cascaded spill merge and re-spill [datafusion]
qstommyshu commented on code in PR #15610:
URL: https://github.com/apache/datafusion/pull/15610#discussion_r2040791232
##
datafusion/core/tests/memory_limit/mod.rs:
##
@@ -615,6 +616,104 @@ async fn test_disk_spill_limit_not_reached() ->
Result<()> {
Ok(())
}
+// Test configuration `sort_max_spill_merge_degree` in external sorting
+// ---
+
+// Ensure invalid config value of `sort_max_spill_merge_degree` returns error
+#[rstest]
+#[case(0)]
+#[case(1)]
+#[tokio::test]
+async fn test_invalid_sort_max_spill_merge_degree(
+#[case] sort_max_spill_merge_degree: usize,
Review Comment:
this #[case] syntax looks so elegant for writing repetitive tests
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
