You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "zhuqi-lucas (via GitHub)" <gi...@apache.org> on 2025/02/19 06:24:50 UTC
[PR] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
zhuqi-lucas opened a new pull request, #14766:
URL: https://github.com/apache/datafusion/pull/14766
…ory for the result batches
## Which issue does this PR close?
- Closes[ #14751](https://github.com/apache/datafusion/issues/14751)
## Rationale for this change
This is the follow-up for the discussion https://github.com/apache/datafusion/pull/14644#issuecomment-2665614584
Problem
I tried one query and this PR is not working as expected, I specified one query to run under 5GB memory (select * without order requires 7GB) but it's still consuming around 10GB, could you double check? I suspect we missed some details.
It is a problem of datafusion-cli. If datafusion-cli decides to hold all the result batches in memory, it should create a memory consumer for itself and reserve memory for the result batches.
datafusion-cli [calls collect](https://github.com/apache/datafusion/blob/45.0.0/datafusion-cli/src/exec.rs#L249-L254) to hold the entire result set in memory before displaying it, this is unnecessary when maxrows is not unlimited. I've tried the following code to replace the collect call, and the maximum resident set size has reduced to 4.8 GB:
## What changes are included in this PR?
1. Support memory consumer for datafusion-cli itself and reserve memory for the result batches.
2. Support hold the streaming the result set in memory and displaying it when maxrows is not unlimited.
## Are these changes tested?
## Are there any user-facing changes?
It fixed the datafusion-cli memory high usage when we only print part of the huge result.
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#issuecomment-2668905606
Testing result:
```rust
/usr/bin/time -l cargo run --release -- --mem-pool-type fair -m 5G --maxrows 1 -f '/Users/zhuqi/arrow-datafusion/benchmarks/data/external_sort.sql'
Finished `release` profile [optimized] target(s) in 0.26s
Running `/Users/zhuqi/arrow-datafusion/target/release/datafusion-cli --mem-pool-type fair -m 5G --maxrows 1 -f /Users/zhuqi/arrow-datafusion/benchmarks/data/external_sort.sql`
DataFusion CLI v45.0.0
0 row(s) fetched.
Elapsed 0.006 seconds.
+------------+-----------+-----------+--------------+------------+-----------------+------------+-------+------------+--------------+---------------+
| l_orderkey | l_partkey | l_suppkey | l_linenumber | l_quantity | l_extendedprice | l_discount | l_tax | l_shipdate | l_commitdate | l_receiptdate |
+------------+-----------+-----------+--------------+------------+-----------------+------------+-------+------------+--------------+---------------+
| 1 | 1551894 | 76910 | 1 | 17.00 | 33078.94 | 0.04 | 0.02 | 1996-03-13 | 1996-02-12 | 1996-03-22 |
| . |
| . |
| . |
+------------+-----------+-----------+--------------+------------+-----------------+------------+-------+------------+--------------+---------------+
8192 row(s) fetched. (First 1 displayed. Use --maxrows to adjust)
Elapsed 4.129 seconds.
8.16 real 12.72 user 7.25 sys
5292523520 maximum resident set size
0 average shared memory size
0 average unshared data size
0 average unshared stack size
1334371 page reclaims
0 page faults
0 swaps
0 block input operations
0 block output operations
0 messages sent
0 messages received
0 signals received
1830 voluntary context switches
107473 involuntary context switches
296497285174 instructions retired
74662056546 cycles elapsed
5291681480 peak memory footprint
```
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#discussion_r1965375371
##########
datafusion-cli/src/exec.rs:
##########
@@ -247,10 +253,29 @@ pub(super) async fn exec_and_print(
let stream = execute_stream(physical_plan, task_ctx.clone())?;
print_options.print_stream(stream, now).await?;
} else {
- // Bounded stream; collected results are printed after all input consumed.
+ // Bounded stream; collected results size is limited by the maxrows option
let schema = physical_plan.schema();
- let results = collect(physical_plan, task_ctx.clone()).await?;
- adjusted.into_inner().print_batches(schema, &results, now)?;
+ let mut stream = execute_stream(physical_plan, task_ctx.clone())?;
+ let mut results = vec![];
+ let mut row_count = 0_usize;
+ while let Some(batch) = stream.next().await {
+ let batch = batch?;
+ let curr_num_rows = batch.num_rows();
+ if let MaxRows::Limited(max_rows) = print_options.maxrows {
+ // Stop collecting results if the number of rows exceeds the limit
+ // results batch should include the last batch that exceeds the limit
+ if row_count < max_rows + curr_num_rows {
+ // Try to grow the reservation to accommodate the batch in memory
+ reservation.try_grow(get_record_batch_memory_size(&batch))?;
+ results.push(batch);
+ }
+ }
+ row_count += curr_num_rows;
+ }
+ adjusted
+ .into_inner()
+ .print_batches(schema, &results, now, row_count)?;
Review Comment:
Created a follow-up ticket:
https://github.com/apache/datafusion/issues/14810
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#issuecomment-2670405399
> Don't break early makes sense to me, I believe it's intended in most cases, let's keep it simple.
>
> If the query is selecting the whole table, this result does not look correct:
>
> ```
> 8192 row(s) fetched. (First 1 displayed. Use --maxrows to adjust)
> ```
>
> It should reflect the result size, so it should be the row count of the whole table.
Good catch, @2010YOUY01 , because we don't pass the row-count to print details, i will try to fix it.
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#discussion_r1965369704
##########
datafusion-cli/src/exec.rs:
##########
@@ -247,10 +253,29 @@ pub(super) async fn exec_and_print(
let stream = execute_stream(physical_plan, task_ctx.clone())?;
print_options.print_stream(stream, now).await?;
} else {
- // Bounded stream; collected results are printed after all input consumed.
+ // Bounded stream; collected results size is limited by the maxrows option
let schema = physical_plan.schema();
- let results = collect(physical_plan, task_ctx.clone()).await?;
- adjusted.into_inner().print_batches(schema, &results, now)?;
+ let mut stream = execute_stream(physical_plan, task_ctx.clone())?;
+ let mut results = vec![];
+ let mut row_count = 0_usize;
+ while let Some(batch) = stream.next().await {
+ let batch = batch?;
+ let curr_num_rows = batch.num_rows();
+ if let MaxRows::Limited(max_rows) = print_options.maxrows {
+ // Stop collecting results if the number of rows exceeds the limit
+ // results batch should include the last batch that exceeds the limit
+ if row_count < max_rows + curr_num_rows {
+ // Try to grow the reservation to accommodate the batch in memory
+ reservation.try_grow(get_record_batch_memory_size(&batch))?;
+ results.push(batch);
+ }
+ }
+ row_count += curr_num_rows;
+ }
+ adjusted
+ .into_inner()
+ .print_batches(schema, &results, now, row_count)?;
Review Comment:
Thanks @Dandandan review , it's a good point if we setting the maxrows huge number because the results only keep the maxrows count batch 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: 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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#issuecomment-2667987964
> Maybe we can do more smart things for memory usage for keeping the result and print? For example, we add a option that peek-memory-for-batch-result.
>
> Also we can register the peek-memory-for-batch-result to memory-pool? So we will not register higher memory for datafusion-cli, if we want to fully materialized but we only want to print it?
>
> ```
> current_total_usage
>
> we only register current_total_usage to memory pool, but not fully materialized result
>
> add new batch to current_total_usage
>
> if current_total_usage + batch >= memory-for-batch-result {
> 1. // we should print it
> 2. // we should free the current_total_usage
> }
> ```
I think it's very rare to print super large results to the terminal, to make this approach useful 🤔
I've reconsidered `--retain-full-results`, this is only useful for some very rare profiling use cases, perhaps we can ignore this at the moment for simplicity.
How about only adding `--stop-after-max-rows`, and by default free the memory of evaluated batches after `maxrows` reached?
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#discussion_r1963396925
##########
datafusion-cli/src/print_options.rs:
##########
@@ -102,14 +102,14 @@ impl PrintOptions {
schema: SchemaRef,
batches: &[RecordBatch],
query_start_time: Instant,
+ row_count: usize,
Review Comment:
this is a great idea
##########
datafusion-cli/src/main.rs:
##########
@@ -121,6 +121,13 @@ struct Args {
)]
maxrows: MaxRows,
+ #[clap(
+ short,
+ long,
+ help = "Whether to stop early when max rows is reached, this will help reduce the memory usage when the result is large"
Review Comment:
The option appears to still be present
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#issuecomment-2667753322
Thank you for the help.
This change will stop execution once `maxrow` is reached. I think this is the optimal behavior for application developers using `datafusion-cli` for quick experiments.
However, datafusion internal developers might use it for timing certain queries, so perhaps they want queries to run till the end. Also, I think the current behavior can also be useful, if it is intended to measure the maximum memory footprint with the result fully materialized. (Though it's a rarer case)
So I think we need two extra configurations for this purpose:
`--stop-after-max-rows, default: false` -- Controls whether to stop early when `maxrows` is reached. I guess there is less people using `datafusion-cli` for application purposes? So default to false for accurate timing.
`--retain-full-results, default: false` -- Controls whether to throw away or accumulate result batches after `maxrows` is reached
Now I'm not sure if it's okay to register result sink for a `MemoryReservation`, I'll think about it in the background for a while.
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#issuecomment-2668188668
Addressed comments in latest PR @2010YOUY01 ,thanks!
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#discussion_r1965294394
##########
datafusion-cli/src/exec.rs:
##########
@@ -247,10 +253,29 @@ pub(super) async fn exec_and_print(
let stream = execute_stream(physical_plan, task_ctx.clone())?;
print_options.print_stream(stream, now).await?;
} else {
- // Bounded stream; collected results are printed after all input consumed.
+ // Bounded stream; collected results size is limited by the maxrows option
let schema = physical_plan.schema();
- let results = collect(physical_plan, task_ctx.clone()).await?;
- adjusted.into_inner().print_batches(schema, &results, now)?;
+ let mut stream = execute_stream(physical_plan, task_ctx.clone())?;
+ let mut results = vec![];
+ let mut row_count = 0_usize;
+ while let Some(batch) = stream.next().await {
+ let batch = batch?;
+ let curr_num_rows = batch.num_rows();
+ if let MaxRows::Limited(max_rows) = print_options.maxrows {
+ // Stop collecting results if the number of rows exceeds the limit
+ // results batch should include the last batch that exceeds the limit
+ if row_count < max_rows + curr_num_rows {
+ // Try to grow the reservation to accommodate the batch in memory
+ reservation.try_grow(get_record_batch_memory_size(&batch))?;
+ results.push(batch);
+ }
+ }
+ row_count += curr_num_rows;
+ }
+ adjusted
+ .into_inner()
+ .print_batches(schema, &results, now, row_count)?;
Review Comment:
Would be nice if it even could print based on the stream instead of collecting.
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 merged PR #14766:
URL: https://github.com/apache/datafusion/pull/14766
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#issuecomment-2668448654
> It is a problem of datafusion-cli. If datafusion-cli decides to hold all the result batches in memory, it should create a memory consumer for itself and reserve memory for the result batches.
Another thing that I remember `psql` did was to only buffer the first like 1000 lines or so to figure out column spacing -- right now `datafusion-cli` buffers the entire thing to ensure it can format all the rows reasonably which is likely unecessary for the majority of the time
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#issuecomment-2667614609
cc @alamb @2010YOUY01
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#discussion_r1961557419
##########
datafusion-cli/src/main.rs:
##########
@@ -121,6 +121,13 @@ struct Args {
)]
maxrows: MaxRows,
+ #[clap(
+ short,
+ long,
+ help = "Whether to stop early when max rows is reached, this will help reduce the memory usage when the result is large"
Review Comment:
I think this option is basically the same as adding a `LIMIT max_rows` to the query and thus is somewhat redundant
##########
datafusion-cli/src/exec.rs:
##########
@@ -249,8 +255,21 @@ pub(super) async fn exec_and_print(
} else {
// Bounded stream; collected results are printed after all input consumed.
let schema = physical_plan.schema();
- let results = collect(physical_plan, task_ctx.clone()).await?;
+ let mut stream = execute_stream(physical_plan, task_ctx.clone())?;
+ let mut results = vec![];
+ while let Some(batch) = stream.next().await {
+ let batch = batch?;
+ reservation.try_grow(get_record_batch_memory_size(&batch))?;
+ results.push(batch);
+ if let MaxRows::Limited(max_rows) = print_options.maxrows {
+ // Stop collecting results if the number of rows exceeds the limit
+ if results.len() >= max_rows && print_options.stop_after_max_rows {
Review Comment:
If you break here it will stop the plan early -- then you need to add `stop_after_max_rows` to permit the plan to run to completion, however, if that option is set then the data is buffered again (which from a memory perspective is the same as increasing the max_rows)
Rather than `break`ing here, I recommend just ignoring the batches after `max_rows` has been reached (aka don't `push` them into `results`) -- I think that would be the most intuitive behavior:
1. Queries would still always run to completion
2. Max rows would control how much memory was used by datafusion-cli buffering
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#issuecomment-2668192400
I think, it will also benefit the https://github.com/apache/datafusion/issues/14510
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#issuecomment-2667636669
Before this PR, the result is double:
```rust
/usr/bin/time -l cargo run --release -- --mem-pool-type fair -m 5G --maxrows 10 -f '/Users/zhuqi/arrow-datafusion/benchmarks/data/external_sort.sql'
Compiling datafusion-cli v45.0.0 (/Users/zhuqi/arrow-datafusion/datafusion-cli)
Finished `release` profile [optimized] target(s) in 6m 05s
Running `/Users/zhuqi/arrow-datafusion/target/release/datafusion-cli --mem-pool-type fair -m 5G --maxrows 10 -f /Users/zhuqi/arrow-datafusion/benchmarks/data/external_sort.sql`
DataFusion CLI v45.0.0
0 row(s) fetched.
Elapsed 0.007 seconds.
+------------+-----------+-----------+--------------+------------+-----------------+------------+-------+------------+--------------+---------------+
| l_orderkey | l_partkey | l_suppkey | l_linenumber | l_quantity | l_extendedprice | l_discount | l_tax | l_shipdate | l_commitdate | l_receiptdate |
+------------+-----------+-----------+--------------+------------+-----------------+------------+-------+------------+--------------+---------------+
| 1 | 1551894 | 76910 | 1 | 17.00 | 33078.94 | 0.04 | 0.02 | 1996-03-13 | 1996-02-12 | 1996-03-22 |
| 1 | 673091 | 73092 | 2 | 36.00 | 38306.16 | 0.09 | 0.06 | 1996-04-12 | 1996-02-28 | 1996-04-20 |
| 1 | 636998 | 36999 | 3 | 8.00 | 15479.68 | 0.10 | 0.02 | 1996-01-29 | 1996-03-05 | 1996-01-31 |
| 1 | 21315 | 46316 | 4 | 28.00 | 34616.68 | 0.09 | 0.06 | 1996-04-21 | 1996-03-30 | 1996-05-16 |
| 1 | 240267 | 15274 | 5 | 24.00 | 28974.00 | 0.10 | 0.04 | 1996-03-30 | 1996-03-14 | 1996-04-01 |
| 1 | 156345 | 6348 | 6 | 32.00 | 44842.88 | 0.07 | 0.02 | 1996-01-30 | 1996-02-07 | 1996-02-03 |
| 2 | 1061698 | 11719 | 1 | 38.00 | 63066.32 | 0.00 | 0.05 | 1997-01-28 | 1997-01-14 | 1997-02-02 |
| 3 | 42970 | 17971 | 1 | 45.00 | 86083.65 | 0.06 | 0.00 | 1994-02-02 | 1994-01-04 | 1994-02-23 |
| 3 | 190355 | 65359 | 2 | 49.00 | 70822.15 | 0.10 | 0.00 | 1993-11-09 | 1993-12-20 | 1993-11-24 |
| 3 | 1284483 | 34508 | 3 | 27.00 | 39620.34 | 0.06 | 0.07 | 1994-01-16 | 1993-11-22 | 1994-01-23 |
| . |
| . |
| . |
+------------+-----------+-----------+--------------+------------+-----------------+------------+-------+------------+--------------+---------------+
59986052 row(s) fetched. (First 10 displayed. Use --maxrows to adjust)
Elapsed 5.044 seconds.
375.80 real 13.14 user 8.05 sys
9792942080 maximum resident set size
0 average shared memory size
0 average unshared data size
0 average unshared stack size
2445044 page reclaims
205 page faults
0 swaps
0 block input operations
0 block output operations
0 messages sent
0 messages received
0 signals received
2537 voluntary context switches
99881 involuntary context switches
313190538941 instructions retired
79238532090 cycles elapsed
9802777552 peak memory footprint
```
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#discussion_r1961832072
##########
datafusion-cli/src/main.rs:
##########
@@ -121,6 +121,13 @@ struct Args {
)]
maxrows: MaxRows,
+ #[clap(
+ short,
+ long,
+ help = "Whether to stop early when max rows is reached, this will help reduce the memory usage when the result is large"
Review Comment:
Very good point, remove it now.
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#discussion_r1961835327
##########
datafusion-cli/src/exec.rs:
##########
@@ -249,8 +255,21 @@ pub(super) async fn exec_and_print(
} else {
// Bounded stream; collected results are printed after all input consumed.
let schema = physical_plan.schema();
- let results = collect(physical_plan, task_ctx.clone()).await?;
+ let mut stream = execute_stream(physical_plan, task_ctx.clone())?;
+ let mut results = vec![];
+ while let Some(batch) = stream.next().await {
+ let batch = batch?;
+ reservation.try_grow(get_record_batch_memory_size(&batch))?;
+ results.push(batch);
+ if let MaxRows::Limited(max_rows) = print_options.maxrows {
+ // Stop collecting results if the number of rows exceeds the limit
+ if results.len() >= max_rows && print_options.stop_after_max_rows {
Review Comment:
Thank you @alamb for review and the great idea! It makes sense to me, addressed in latest PR.
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#discussion_r1961553561
##########
datafusion-cli/src/exec.rs:
##########
@@ -249,8 +255,21 @@ pub(super) async fn exec_and_print(
} else {
// Bounded stream; collected results are printed after all input consumed.
let schema = physical_plan.schema();
- let results = collect(physical_plan, task_ctx.clone()).await?;
+ let mut stream = execute_stream(physical_plan, task_ctx.clone())?;
+ let mut results = vec![];
+ while let Some(batch) = stream.next().await {
+ let batch = batch?;
+ reservation.try_grow(get_record_batch_memory_size(&batch))?;
+ results.push(batch);
+ if let MaxRows::Limited(max_rows) = print_options.maxrows {
+ // Stop collecting results if the number of rows exceeds the limit
Review Comment:
this is a great idea
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#issuecomment-2670429714
Fixed the row count in latest PR, thanks @2010YOUY01 ! It's the same count before this PR.
```rust
/usr/bin/time -l cargo run --release -- --mem-pool-type fair -m 5G --maxrows 1 -f '/Users/zhuqi/arrow-datafusion/benchmarks/data/external_sort.sql'
Compiling datafusion-cli v45.0.0 (/Users/zhuqi/arrow-datafusion/datafusion-cli)
Building [=======================> ] 541/542: datafusion-cli(bin)
Finished `release` profile [optimized] target(s) in 6m 08s
Running `/Users/zhuqi/arrow-datafusion/target/release/datafusion-cli --mem-pool-type fair -m 5G --maxrows 1 -f /Users/zhuqi/arrow-datafusion/benchmarks/data/external_sort.sql`
DataFusion CLI v45.0.0
0 row(s) fetched.
Elapsed 0.006 seconds.
+------------+-----------+-----------+--------------+------------+-----------------+------------+-------+------------+--------------+---------------+
| l_orderkey | l_partkey | l_suppkey | l_linenumber | l_quantity | l_extendedprice | l_discount | l_tax | l_shipdate | l_commitdate | l_receiptdate |
+------------+-----------+-----------+--------------+------------+-----------------+------------+-------+------------+--------------+---------------+
+------------+-----------+-----------+--------------+------------+-----------------+------------+-------+------------+--------------+---------------+
59986052 row(s) fetched. (First 1 displayed. Use --maxrows to adjust)
Elapsed 4.149 seconds.
376.95 real 12.53 user 6.73 sys
5150486528 maximum resident set size
0 average shared memory size
0 average unshared data size
0 average unshared stack size
1310769 page reclaims
0 page faults
0 swaps
0 block input operations
0 block output operations
0 messages sent
0 messages received
0 signals received
1879 voluntary context switches
96870 involuntary context switches
292528936267 instructions retired
72204445152 cycles elapsed
5149381912 peak memory footprint
```
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#issuecomment-2668895304
> Thank you for working on this @zhuqi-lucas -- I had some comments. Let me know what you think
Great idea, thank you @alamb , addressed in latest PR.
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#discussion_r1961553561
##########
datafusion-cli/src/exec.rs:
##########
@@ -249,8 +255,21 @@ pub(super) async fn exec_and_print(
} else {
// Bounded stream; collected results are printed after all input consumed.
let schema = physical_plan.schema();
- let results = collect(physical_plan, task_ctx.clone()).await?;
+ let mut stream = execute_stream(physical_plan, task_ctx.clone())?;
+ let mut results = vec![];
+ while let Some(batch) = stream.next().await {
+ let batch = batch?;
+ reservation.try_grow(get_record_batch_memory_size(&batch))?;
+ results.push(batch);
+ if let MaxRows::Limited(max_rows) = print_options.maxrows {
+ // Stop collecting results if the number of rows exceeds the limit
Review Comment:
this is a great idea
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#discussion_r1965517356
##########
datafusion-cli/src/exec.rs:
##########
@@ -247,10 +253,29 @@ pub(super) async fn exec_and_print(
let stream = execute_stream(physical_plan, task_ctx.clone())?;
print_options.print_stream(stream, now).await?;
} else {
- // Bounded stream; collected results are printed after all input consumed.
+ // Bounded stream; collected results size is limited by the maxrows option
let schema = physical_plan.schema();
- let results = collect(physical_plan, task_ctx.clone()).await?;
- adjusted.into_inner().print_batches(schema, &results, now)?;
+ let mut stream = execute_stream(physical_plan, task_ctx.clone())?;
+ let mut results = vec![];
+ let mut row_count = 0_usize;
+ while let Some(batch) = stream.next().await {
+ let batch = batch?;
+ let curr_num_rows = batch.num_rows();
+ if let MaxRows::Limited(max_rows) = print_options.maxrows {
+ // Stop collecting results if the number of rows exceeds the limit
+ // results batch should include the last batch that exceeds the limit
+ if row_count < max_rows + curr_num_rows {
+ // Try to grow the reservation to accommodate the batch in memory
+ reservation.try_grow(get_record_batch_memory_size(&batch))?;
+ results.push(batch);
+ }
+ }
+ row_count += curr_num_rows;
+ }
+ adjusted
+ .into_inner()
+ .print_batches(schema, &results, now, row_count)?;
Review Comment:
Nice
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#issuecomment-2670371222
Don't break early makes sense to me, I believe it's intended in most cases, let's keep it simple.
If the query is selecting the whole table, this result does not look correct:
```
8192 row(s) fetched. (First 1 displayed. Use --maxrows to adjust)
```
It should reflect the result size, so it should be the row count of the whole table.
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#issuecomment-2667624674
1. The memory usage now is accurate, it will not collect all result to memory.
2. We now register datafusion-cli result batch to memory pool also.
The testing result for the 10G memory case, now it's 5G peak memory:
```rust
/usr/bin/time -l cargo run --release -- --mem-pool-type fair -m 5G --maxrows 10 -f '/Users/zhuqi/arrow-datafusion/benchmarks/data/external_sort.sql'
Compiling datafusion-cli v45.0.0 (/Users/zhuqi/arrow-datafusion/datafusion-cli)
Finished `release` profile [optimized] target(s) in 6m 06s
Running `/Users/zhuqi/arrow-datafusion/target/release/datafusion-cli --mem-pool-type fair -m 5G --maxrows 10 -f /Users/zhuqi/arrow-datafusion/benchmarks/data/external_sort.sql`
DataFusion CLI v45.0.0
0 row(s) fetched.
Elapsed 0.006 seconds.
memory pool: FairSpillPool { pool_size: 5368709120, state: Mutex { data: FairSpillPoolState { num_spill: 0, spillable: 0, unspillable: 0 } } }
+------------+-----------+-----------+--------------+------------+-----------------+------------+-------+------------+--------------+---------------+
| l_orderkey | l_partkey | l_suppkey | l_linenumber | l_quantity | l_extendedprice | l_discount | l_tax | l_shipdate | l_commitdate | l_receiptdate |
+------------+-----------+-----------+--------------+------------+-----------------+------------+-------+------------+--------------+---------------+
| 1 | 1551894 | 76910 | 1 | 17.00 | 33078.94 | 0.04 | 0.02 | 1996-03-13 | 1996-02-12 | 1996-03-22 |
| 1 | 673091 | 73092 | 2 | 36.00 | 38306.16 | 0.09 | 0.06 | 1996-04-12 | 1996-02-28 | 1996-04-20 |
| 1 | 636998 | 36999 | 3 | 8.00 | 15479.68 | 0.10 | 0.02 | 1996-01-29 | 1996-03-05 | 1996-01-31 |
| 1 | 21315 | 46316 | 4 | 28.00 | 34616.68 | 0.09 | 0.06 | 1996-04-21 | 1996-03-30 | 1996-05-16 |
| 1 | 240267 | 15274 | 5 | 24.00 | 28974.00 | 0.10 | 0.04 | 1996-03-30 | 1996-03-14 | 1996-04-01 |
| 1 | 156345 | 6348 | 6 | 32.00 | 44842.88 | 0.07 | 0.02 | 1996-01-30 | 1996-02-07 | 1996-02-03 |
| 2 | 1061698 | 11719 | 1 | 38.00 | 63066.32 | 0.00 | 0.05 | 1997-01-28 | 1997-01-14 | 1997-02-02 |
| 3 | 42970 | 17971 | 1 | 45.00 | 86083.65 | 0.06 | 0.00 | 1994-02-02 | 1994-01-04 | 1994-02-23 |
| 3 | 190355 | 65359 | 2 | 49.00 | 70822.15 | 0.10 | 0.00 | 1993-11-09 | 1993-12-20 | 1993-11-24 |
| 3 | 1284483 | 34508 | 3 | 27.00 | 39620.34 | 0.06 | 0.07 | 1994-01-16 | 1993-11-22 | 1994-01-23 |
| . |
| . |
| . |
+------------+-----------+-----------+--------------+------------+-----------------+------------+-------+------------+--------------+---------------+
81920 row(s) fetched. (First 10 displayed. Use --maxrows to adjust)
Elapsed 2.165 seconds.
373.37 real 9.21 user 5.91 sys
5073829888 maximum resident set size
0 average shared memory size
0 average unshared data size
0 average unshared stack size
1293674 page reclaims
0 page faults
0 swaps
0 block input operations
0 block output operations
0 messages sent
0 messages received
0 signals received
1906 voluntary context switches
85462 involuntary context switches
200845261488 instructions retired
55793294693 cycles elapsed
5072421856 peak memory footprint
```
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#issuecomment-2667860443
Thank you @2010YOUY01 for review, good suggestion, if i make sense right, so we will control two extra things:
1. --stop-after-max-rows, default: false , we can use this to control the end of the streaming.
```rust
if results.len() >= max_rows && stop-after-max-rows{
break;
}
```
2. --retain-full-results, default: false , we can use this to control the memory usage:
Maybe we can get more smart for memory usage for keeping the result and print? For example, we add a option that peek-memory-for-batch-result.
Also we can register the peek-memory-for-batch-result to memory-pool?
```
current_total_usage
if current_total_usage + batch >= memory-for-batch-result {
1. // we should print it
2. // we should free the current_total_usage
}
```
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#issuecomment-2671279588
> Thanks @zhuqi-lucas and @2010YOUY01
>
> I think it would be nice to remove the unused `stop_after_max_rows` option now, but we could also do it as a follow on PR too if you prefer
Thank you @alamb @2010YOUY01 for review, also removed the unused stop_after_max_rows option in latest PR!
--
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] feat: Improve datafusion-cli memory usage and considering reserve mem… [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #14766:
URL: https://github.com/apache/datafusion/pull/14766#issuecomment-2668029422
> > Maybe we can do more smart things for memory usage for keeping the result and print? For example, we add a option that peek-memory-for-batch-result.
> > Also we can register the peek-memory-for-batch-result to memory-pool? So we will not register higher memory for datafusion-cli, if we want to fully materialized but we only want to print it?
> > ```
> > current_total_usage
> >
> > we only register current_total_usage to memory pool, but not fully materialized result
> >
> > add new batch to current_total_usage
> >
> > if current_total_usage + batch >= memory-for-batch-result {
> > 1. // we should print it
> > 2. // we should free the current_total_usage
> > }
> > ```
>
> I think it's very rare to print super large results to the terminal, to make this approach useful 🤔 I've reconsidered `--retain-full-results`, this is only useful for some very rare profiling use cases, perhaps we can ignore this at the moment for simplicity. How about only adding `--stop-after-max-rows`, and by default free the memory of evaluated batches after `maxrows` reached?
@2010YOUY01 It makes sense to me, thanks!
--
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