Re: [PR] feat: support inability to yield for loop when it's not using Tok… [datafusion]

2025-05-28 Thread via GitHub 


zhuqi-lucas commented on PR #16196:
URL: https://github.com/apache/datafusion/pull/16196#issuecomment-2918118294

   > 🤖: Benchmark completed
   > 
   > Details
   > 
   > ```
   > Comparing HEAD and issue_16193
   > 
   > Benchmark clickbench_extended.json
   > 
   > ┏━━┳┳━┳━━━┓
   > ┃ Query┃   HEAD ┃ issue_16193 ┃Change ┃
   > ┡━━╇╇━╇━━━┩
   > │ QQuery 0 │  1906.70ms │   1850.03ms │ no change │
   > │ QQuery 1 │   691.18ms │691.76ms │ no change │
   > │ QQuery 2 │  1411.18ms │   1436.19ms │ no change │
   > │ QQuery 3 │   696.17ms │700.91ms │ no change │
   > │ QQuery 4 │  1438.68ms │   1458.59ms │ no change │
   > │ QQuery 5 │ 15100.87ms │  14874.14ms │ no change │
   > │ QQuery 6 │  1996.44ms │   1982.89ms │ no change │
   > │ QQuery 7 │  2089.84ms │   2063.71ms │ no change │
   > │ QQuery 8 │   830.88ms │835.14ms │ no change │
   > └──┴┴─┴───┘
   > ┏┳┓
   > ┃ Benchmark Summary  ┃┃
   > ┡╇┩
   > │ Total Time (HEAD)  │ 26161.93ms │
   > │ Total Time (issue_16193)   │ 25893.35ms │
   > │ Average Time (HEAD)│  2906.88ms │
   > │ Average Time (issue_16193) │  2877.04ms │
   > │ Queries Faster │  0 │
   > │ Queries Slower │  0 │
   > │ Queries with No Change │  9 │
   > └┴┘
   > 
   > Benchmark clickbench_partitioned.json
   > 
   > ┏━━┳┳━┳━━━┓
   > ┃ Query┃   HEAD ┃ issue_16193 ┃Change ┃
   > ┡━━╇╇━╇━━━┩
   > │ QQuery 0 │15.18ms │ 15.80ms │ no change │
   > │ QQuery 1 │33.16ms │ 32.97ms │ no change │
   > │ QQuery 2 │77.60ms │ 80.92ms │ no change │
   > │ QQuery 3 │97.69ms │ 97.56ms │ no change │
   > │ QQuery 4 │   589.28ms │594.05ms │ no change │
   > │ QQuery 5 │   847.85ms │836.44ms │ no change │
   > │ QQuery 6 │23.38ms │ 22.84ms │ no change │
   > │ QQuery 7 │39.02ms │ 36.26ms │ +1.08x faster │
   > │ QQuery 8 │   923.80ms │927.93ms │ no change │
   > │ QQuery 9 │  1194.31ms │   1194.76ms │ no change │
   > │ QQuery 10│   261.49ms │264.83ms │ no change │
   > │ QQuery 11│   298.34ms │300.14ms │ no change │
   > │ QQuery 12│   889.86ms │893.87ms │ no change │
   > │ QQuery 13│  1343.97ms │   1319.24ms │ no change │
   > │ QQuery 14│   850.37ms │847.33ms │ no change │
   > │ QQuery 15│   829.14ms │827.15ms │ no change │
   > │ QQuery 16│  1756.84ms │   1712.96ms │ no change │
   > │ QQuery 17│  1607.84ms │   1612.75ms │ no change │
   > │ QQuery 18│  3257.72ms │   3059.25ms │ +1.06x faster │
   > │ QQuery 19│84.43ms │ 81.15ms │ no change │
   > │ QQuery 20│  1143.94ms │   1098.25ms │ no change │
   > │ QQuery 21│  1299.30ms │   1302.19ms │ no change │
   > │ QQuery 22│  2139.01ms │   2154.26ms │ no change │
   > │ QQuery 23│  7959.94ms │   7886.41ms │ no change │
   > │ QQuery 24│   463.30ms │458.37ms │ no change │
   > │ QQuery 25│   386.27ms │380.85ms │ no change │
   > │ QQuery 26│   524.88ms │519.71ms │ no change │
   > │ QQuery 27│  1581.12ms │   1583.97ms │ no change │
   > │ QQuery 28│ 12730.61ms │  12509.52ms │ no change │
   > │ QQuery 29│   536.15ms │528.59ms │ no change │
   > │ QQuery 30│   793.18ms │795.03ms │ no change │
   > │ QQuery 31│   857.17ms │852.83ms │ no change │
   > │ QQuery 32│  2653.95ms │   2631.77ms │ no change │
   > │ QQuery 33│  3315.12ms │   3322.64ms │ no change │
   > │ QQuery 34│  3381.42ms │   3355.04ms │ no change │
   > │ QQuery 35│  1317.57ms │   1278.45ms │ no change │
   > │ QQuery 36│   129.98ms │122.68ms │ +1.06x faster │
   > │ QQuery 37│56.20ms │ 53.71ms │ no change │
   > │ QQuery 38│   119.55ms │121.04ms │ no change │
   > │ QQuery 39│   193.87ms │194.76ms │ no change │
   > │ QQuery 40│48.93ms │ 49.63ms │ no change │
   > │ QQuery 41│44.38ms │ 45.77ms │ no change │
   > │ QQuery 42│36.47ms │ 37.46ms │ no change │
   > └──┴┴─┴───┘
   > ┏┳┓
   > ┃ Benchmark Summary  ┃┃
   > ┡╇┩
   > │ Total Time (HEAD)  │ 56733.55ms │
   > │ Total Ti

Re: [PR] feat: support inability to yield for loop when it's not using Tok… [datafusion]

2025-05-28 Thread via GitHub 


alamb commented on PR #16196:
URL: https://github.com/apache/datafusion/pull/16196#issuecomment-2917443275

   🤖: Benchmark completed
   
   Details
   
   
   
   ```
   Comparing HEAD and issue_16193
   
   Benchmark clickbench_extended.json
   
   ┏━━┳┳━┳━━━┓
   ┃ Query┃   HEAD ┃ issue_16193 ┃Change ┃
   ┡━━╇╇━╇━━━┩
   │ QQuery 0 │  1906.70ms │   1850.03ms │ no change │
   │ QQuery 1 │   691.18ms │691.76ms │ no change │
   │ QQuery 2 │  1411.18ms │   1436.19ms │ no change │
   │ QQuery 3 │   696.17ms │700.91ms │ no change │
   │ QQuery 4 │  1438.68ms │   1458.59ms │ no change │
   │ QQuery 5 │ 15100.87ms │  14874.14ms │ no change │
   │ QQuery 6 │  1996.44ms │   1982.89ms │ no change │
   │ QQuery 7 │  2089.84ms │   2063.71ms │ no change │
   │ QQuery 8 │   830.88ms │835.14ms │ no change │
   └──┴┴─┴───┘
   ┏┳┓
   ┃ Benchmark Summary  ┃┃
   ┡╇┩
   │ Total Time (HEAD)  │ 26161.93ms │
   │ Total Time (issue_16193)   │ 25893.35ms │
   │ Average Time (HEAD)│  2906.88ms │
   │ Average Time (issue_16193) │  2877.04ms │
   │ Queries Faster │  0 │
   │ Queries Slower │  0 │
   │ Queries with No Change │  9 │
   └┴┘
   
   Benchmark clickbench_partitioned.json
   
   ┏━━┳┳━┳━━━┓
   ┃ Query┃   HEAD ┃ issue_16193 ┃Change ┃
   ┡━━╇╇━╇━━━┩
   │ QQuery 0 │15.18ms │ 15.80ms │ no change │
   │ QQuery 1 │33.16ms │ 32.97ms │ no change │
   │ QQuery 2 │77.60ms │ 80.92ms │ no change │
   │ QQuery 3 │97.69ms │ 97.56ms │ no change │
   │ QQuery 4 │   589.28ms │594.05ms │ no change │
   │ QQuery 5 │   847.85ms │836.44ms │ no change │
   │ QQuery 6 │23.38ms │ 22.84ms │ no change │
   │ QQuery 7 │39.02ms │ 36.26ms │ +1.08x faster │
   │ QQuery 8 │   923.80ms │927.93ms │ no change │
   │ QQuery 9 │  1194.31ms │   1194.76ms │ no change │
   │ QQuery 10│   261.49ms │264.83ms │ no change │
   │ QQuery 11│   298.34ms │300.14ms │ no change │
   │ QQuery 12│   889.86ms │893.87ms │ no change │
   │ QQuery 13│  1343.97ms │   1319.24ms │ no change │
   │ QQuery 14│   850.37ms │847.33ms │ no change │
   │ QQuery 15│   829.14ms │827.15ms │ no change │
   │ QQuery 16│  1756.84ms │   1712.96ms │ no change │
   │ QQuery 17│  1607.84ms │   1612.75ms │ no change │
   │ QQuery 18│  3257.72ms │   3059.25ms │ +1.06x faster │
   │ QQuery 19│84.43ms │ 81.15ms │ no change │
   │ QQuery 20│  1143.94ms │   1098.25ms │ no change │
   │ QQuery 21│  1299.30ms │   1302.19ms │ no change │
   │ QQuery 22│  2139.01ms │   2154.26ms │ no change │
   │ QQuery 23│  7959.94ms │   7886.41ms │ no change │
   │ QQuery 24│   463.30ms │458.37ms │ no change │
   │ QQuery 25│   386.27ms │380.85ms │ no change │
   │ QQuery 26│   524.88ms │519.71ms │ no change │
   │ QQuery 27│  1581.12ms │   1583.97ms │ no change │
   │ QQuery 28│ 12730.61ms │  12509.52ms │ no change │
   │ QQuery 29│   536.15ms │528.59ms │ no change │
   │ QQuery 30│   793.18ms │795.03ms │ no change │
   │ QQuery 31│   857.17ms │852.83ms │ no change │
   │ QQuery 32│  2653.95ms │   2631.77ms │ no change │
   │ QQuery 33│  3315.12ms │   3322.64ms │ no change │
   │ QQuery 34│  3381.42ms │   3355.04ms │ no change │
   │ QQuery 35│  1317.57ms │   1278.45ms │ no change │
   │ QQuery 36│   129.98ms │122.68ms │ +1.06x faster │
   │ QQuery 37│56.20ms │ 53.71ms │ no change │
   │ QQuery 38│   119.55ms │121.04ms │ no change │
   │ QQuery 39│   193.87ms │194.76ms │ no change │
   │ QQuery 40│48.93ms │ 49.63ms │ no change │
   │ QQuery 41│44.38ms │ 45.77ms │ no change │
   │ QQuery 42│36.47ms │ 37.46ms │ no change │
   └──┴┴─┴───┘
   ┏┳┓
   ┃ Benchmark Summary  ┃┃
   ┡╇┩
   │ Total Time (HEAD)  │ 56733.55ms │
   │ Total Time (issue_16193)   │ 56041.14ms │
   │ Average Time (HEAD)│  1319.38ms │
   │ Average Time (issue_16193) │  1303.28ms │
   │ Queries Faster │  3 │

Re: [PR] feat: support inability to yield for loop when it's not using Tok… [datafusion]

2025-05-28 Thread via GitHub 


alamb commented on PR #16196:
URL: https://github.com/apache/datafusion/pull/16196#issuecomment-2917407566

   🤖 `./gh_compare_branch.sh` [Benchmark 
Script](https://github.com/alamb/datafusion-benchmarking/blob/main/gh_compare_branch.sh)
 Running
   Linux aal-dev 6.11.0-1013-gcp #13~24.04.1-Ubuntu SMP Wed Apr  2 16:34:16 UTC 
2025 x86_64 x86_64 x86_64 GNU/Linux
   Comparing issue_16193 (6cf3bf0fbc474a33030184fa7f0f6c013db7c7b7) to 
2d12bf6715e59142594cbc0ccb11bb19e4826b06 
[diff](https://github.com/apache/datafusion/compare/2d12bf6715e59142594cbc0ccb11bb19e4826b06..6cf3bf0fbc474a33030184fa7f0f6c013db7c7b7)
   Benchmarks: tpch_mem clickbench_partitioned clickbench_extended
   Results will be posted here when complete
   


-- 
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] feat: support inability to yield for loop when it's not using Tok… [datafusion]

2025-05-28 Thread via GitHub 


alamb commented on PR #16196:
URL: https://github.com/apache/datafusion/pull/16196#issuecomment-2917408198

   Running the benchmarks again to gather more details


-- 
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] feat: support inability to yield for loop when it's not using Tok… [datafusion]

2025-05-28 Thread via GitHub 


alamb commented on PR #16196:
URL: https://github.com/apache/datafusion/pull/16196#issuecomment-2917351712

   🤖: Benchmark completed
   
   Details
   
   
   
   ```
   Comparing HEAD and issue_16193
   
   Benchmark clickbench_extended.json
   
   ┏━━┳┳━┳━━┓
   ┃ Query┃   HEAD ┃ issue_16193 ┃   Change ┃
   ┡━━╇╇━╇━━┩
   │ QQuery 0 │  1884.53ms │   1932.55ms │no change │
   │ QQuery 1 │   692.42ms │704.34ms │no change │
   │ QQuery 2 │  1422.09ms │   1424.04ms │no change │
   │ QQuery 3 │   727.80ms │722.42ms │no change │
   │ QQuery 4 │  1434.99ms │   1447.24ms │no change │
   │ QQuery 5 │ 15295.77ms │  15299.58ms │no change │
   │ QQuery 6 │  1997.15ms │   2013.51ms │no change │
   │ QQuery 7 │  2049.62ms │   2168.78ms │ 1.06x slower │
   │ QQuery 8 │   836.82ms │848.06ms │no change │
   └──┴┴─┴──┘
   ┏┳┓
   ┃ Benchmark Summary  ┃┃
   ┡╇┩
   │ Total Time (HEAD)  │ 26341.19ms │
   │ Total Time (issue_16193)   │ 26560.52ms │
   │ Average Time (HEAD)│  2926.80ms │
   │ Average Time (issue_16193) │  2951.17ms │
   │ Queries Faster │  0 │
   │ Queries Slower │  1 │
   │ Queries with No Change │  8 │
   └┴┘
   
   Benchmark clickbench_partitioned.json
   
   ┏━━┳┳━┳━━┓
   ┃ Query┃   HEAD ┃ issue_16193 ┃   Change ┃
   ┡━━╇╇━╇━━┩
   │ QQuery 0 │15.54ms │ 15.42ms │no change │
   │ QQuery 1 │33.15ms │ 33.70ms │no change │
   │ QQuery 2 │80.58ms │ 80.48ms │no change │
   │ QQuery 3 │96.06ms │ 95.48ms │no change │
   │ QQuery 4 │   588.87ms │595.63ms │no change │
   │ QQuery 5 │   818.19ms │815.70ms │no change │
   │ QQuery 6 │23.64ms │ 23.47ms │no change │
   │ QQuery 7 │37.27ms │ 36.33ms │no change │
   │ QQuery 8 │   926.36ms │910.32ms │no change │
   │ QQuery 9 │  1187.66ms │   1209.81ms │no change │
   │ QQuery 10│   267.19ms │257.24ms │no change │
   │ QQuery 11│   293.30ms │292.35ms │no change │
   │ QQuery 12│   911.85ms │911.14ms │no change │
   │ QQuery 13│  1247.36ms │   1317.95ms │ 1.06x slower │
   │ QQuery 14│   848.42ms │844.38ms │no change │
   │ QQuery 15│   834.03ms │835.94ms │no change │
   │ QQuery 16│  1736.24ms │   1752.77ms │no change │
   │ QQuery 17│  1613.09ms │   1606.14ms │no change │
   │ QQuery 18│  3084.41ms │   3051.47ms │no change │
   │ QQuery 19│83.51ms │ 84.57ms │no change │
   │ QQuery 20│  1125.18ms │   1120.50ms │no change │
   │ QQuery 21│  1285.82ms │   1308.43ms │no change │
   │ QQuery 22│  2139.79ms │   2162.32ms │no change │
   │ QQuery 23│  8003.46ms │   7992.05ms │no change │
   │ QQuery 24│   463.30ms │450.76ms │no change │
   │ QQuery 25│   384.45ms │381.51ms │no change │
   │ QQuery 26│   521.56ms │522.91ms │no change │
   │ QQuery 27│  1586.93ms │   1590.23ms │no change │
   │ QQuery 28│ 12567.57ms │  12455.67ms │no change │
   │ QQuery 29│   526.70ms │534.84ms │no change │
   │ QQuery 30│   807.81ms │815.44ms │no change │
   │ QQuery 31│   858.67ms │848.84ms │no change │
   │ QQuery 32│  2639.73ms │   2641.91ms │no change │
   │ QQuery 33│  3342.77ms │   3316.54ms │no change │
   │ QQuery 34│  3312.00ms │   3325.35ms │no change │
   │ QQuery 35│  1312.86ms │   1309.17ms │no change │
   │ QQuery 36│   117.39ms │116.89ms │no change │
   │ QQuery 37│55.88ms │ 56.08ms │no change │
   │ QQuery 38│   121.82ms │119.37ms │no change │
   │ QQuery 39│   192.09ms │195.42ms │no change │
   │ QQuery 40│44.75ms │ 49.63ms │ 1.11x slower │
   │ QQuery 41│42.75ms │ 44.34ms │no change │
   │ QQuery 42│38.39ms │ 38.13ms │no change │
   └──┴┴─┴──┘
   ┏┳┓
   ┃ Benchmark Summary  ┃┃
   ┡╇┩
   │ Total Time (HEAD)  │ 56218.38ms │
   │ Total Time (issue_16193)   │ 56166.59ms │
   │ Average Time (HEAD)│  1307.40ms │
   │ Average Time (issue_16193) │  1306.20ms │
   │ Queries Faster │  0 │
   │ Qu

Re: [PR] feat: support inability to yield for loop when it's not using Tok… [datafusion]

2025-05-28 Thread via GitHub 


alamb commented on PR #16196:
URL: https://github.com/apache/datafusion/pull/16196#issuecomment-2917252077

   🤖 `./gh_compare_branch.sh` [Benchmark 
Script](https://github.com/alamb/datafusion-benchmarking/blob/main/gh_compare_branch.sh)
 Running
   Linux aal-dev 6.11.0-1013-gcp #13~24.04.1-Ubuntu SMP Wed Apr  2 16:34:16 UTC 
2025 x86_64 x86_64 x86_64 GNU/Linux
   Comparing issue_16193 (6cf3bf0fbc474a33030184fa7f0f6c013db7c7b7) to 
2d12bf6715e59142594cbc0ccb11bb19e4826b06 
[diff](https://github.com/apache/datafusion/compare/2d12bf6715e59142594cbc0ccb11bb19e4826b06..6cf3bf0fbc474a33030184fa7f0f6c013db7c7b7)
   Benchmarks: tpch_mem clickbench_partitioned clickbench_extended
   Results will be posted here when complete
   


-- 
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] feat: support inability to yield for loop when it's not using Tok… [datafusion]

2025-05-28 Thread via GitHub 


zhuqi-lucas commented on PR #16196:
URL: https://github.com/apache/datafusion/pull/16196#issuecomment-2916219267

   Hi @alamb , i believe we also can do the clickbench benchmark for this PR. 
But i am not confident about the result since it seems we will always add some 
overhead to aggregate. 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: [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] feat: support inability to yield for loop when it's not using Tok… [datafusion]

2025-05-28 Thread via GitHub 


zhuqi-lucas commented on code in PR #16196:
URL: https://github.com/apache/datafusion/pull/16196#discussion_r2111771381


##
datafusion/physical-plan/src/aggregates/no_grouping.rs:
##
@@ -77,6 +77,11 @@ impl AggregateStream {
 let baseline_metrics = BaselineMetrics::new(&agg.metrics, partition);
 let input = agg.input.execute(partition, Arc::clone(&context))?;
 
+// Only wrap no‐grouping aggregates in our YieldStream

Review Comment:
   From testing result, it seems the grouping by cases have some performance 
regression.



-- 
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] feat: support inability to yield for loop when it's not using Tok… [datafusion]

2025-05-28 Thread via GitHub 


zhuqi-lucas commented on PR #16196:
URL: https://github.com/apache/datafusion/pull/16196#issuecomment-2916186751

   Another solution is using CoalescePartitionsExec to wrapper:
   
   
   ```rust
   diff --git a/datafusion/physical-plan/src/coalesce_partitions.rs 
b/datafusion/physical-plan/src/coalesce_partitions.rs
   index 114f83068..ffb24463e 100644
   --- a/datafusion/physical-plan/src/coalesce_partitions.rs
   +++ b/datafusion/physical-plan/src/coalesce_partitions.rs
   @@ -154,10 +154,10 @@ impl ExecutionPlan for CoalescePartitionsExec {
0 => internal_err!(
"CoalescePartitionsExec requires at least one input 
partition"
),
   -1 => {
   -// bypass any threading / metrics if there is a single 
partition
   -self.input.execute(0, context)
   -}
   +// 1 => {
   +// // bypass any threading / metrics if there is a single 
partition
   +// self.input.execute(0, context)
   +// }
_ => {
let baseline_metrics = BaselineMetrics::new(&self.metrics, 
partition);
// record the (very) minimal work done so that
   diff --git a/datafusion/physical-plan/src/execution_plan.rs 
b/datafusion/physical-plan/src/execution_plan.rs
   index b81b3c8be..8bb8b2145 100644
   --- a/datafusion/physical-plan/src/execution_plan.rs
   +++ b/datafusion/physical-plan/src/execution_plan.rs
   @@ -963,8 +963,7 @@ pub fn execute_stream(
) -> Result {
match plan.output_partitioning().partition_count() {
0 => Ok(Box::pin(EmptyRecordBatchStream::new(plan.schema(,
   -1 => plan.execute(0, context),
   -2.. => {
   +1.. => {
// merge into a single partition
let plan = CoalescePartitionsExec::new(Arc::clone(&plan));
// CoalescePartitionsExec must produce a single partition
   diff --git a/parquet-testing b/parquet-testing
   index 6e851ddd7..107b36603 16
   --- a/parquet-testing
   +++ b/parquet-testing
   @@ -1 +1 @@
   -Subproject commit 6e851ddd768d6af741c7b15dc594874399fc3cff
   +Subproject commit 107b36603e051aee26bd93e04b871034f6c756c0
   
   ```


-- 
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] feat: support inability to yield for loop when it's not using Tok… [datafusion]

2025-05-28 Thread via GitHub 


zhuqi-lucas commented on code in PR #16196:
URL: https://github.com/apache/datafusion/pull/16196#discussion_r2111685115


##
datafusion/physical-plan/src/aggregates/no_grouping.rs:
##
@@ -77,6 +77,11 @@ impl AggregateStream {
 let baseline_metrics = BaselineMetrics::new(&agg.metrics, partition);
 let input = agg.input.execute(partition, Arc::clone(&context))?;
 
+// Only wrap no‐grouping aggregates in our YieldStream

Review Comment:
   Thank you @pepijnve , i also added the grouping support 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: [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] feat: support inability to yield for loop when it's not using Tok… [datafusion]

2025-05-28 Thread via GitHub 


zhuqi-lucas commented on code in PR #16196:
URL: https://github.com/apache/datafusion/pull/16196#discussion_r2111675314


##
datafusion/physical-plan/src/aggregates/no_grouping.rs:
##
@@ -77,6 +77,11 @@ impl AggregateStream {
 let baseline_metrics = BaselineMetrics::new(&agg.metrics, partition);
 let input = agg.input.execute(partition, Arc::clone(&context))?;
 
+// Only wrap no‐grouping aggregates in our YieldStream

Review Comment:
   You are right, i can reproduce it now:
   
   ```rust
   SELECT
 (value % 10) AS group_key,
 COUNT(*) AS cnt,
 SUM(value)   AS sum_val
   FROM range(1, 50) AS t
   GROUP BY (value % 10)
   ORDER BY group_key;
   ```



-- 
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] feat: support inability to yield for loop when it's not using Tok… [datafusion]

2025-05-28 Thread via GitHub 


zhuqi-lucas commented on code in PR #16196:
URL: https://github.com/apache/datafusion/pull/16196#discussion_r2111658311


##
datafusion/physical-plan/src/aggregates/no_grouping.rs:
##
@@ -77,6 +77,11 @@ impl AggregateStream {
 let baseline_metrics = BaselineMetrics::new(&agg.metrics, partition);
 let input = agg.input.execute(partition, Arc::clone(&context))?;
 
+// Only wrap no‐grouping aggregates in our YieldStream

Review Comment:
   Thank you @pepijnve for review, i will try to reproduce it for group by 
aggregates.



-- 
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] feat: support inability to yield for loop when it's not using Tok… [datafusion]

2025-05-28 Thread via GitHub 


pepijnve commented on code in PR #16196:
URL: https://github.com/apache/datafusion/pull/16196#discussion_r2111655000


##
datafusion/physical-plan/src/aggregates/no_grouping.rs:
##
@@ -77,6 +77,11 @@ impl AggregateStream {
 let baseline_metrics = BaselineMetrics::new(&agg.metrics, partition);
 let input = agg.input.execute(partition, Arc::clone(&context))?;
 
+// Only wrap no‐grouping aggregates in our YieldStream

Review Comment:
   In my own testing with `partition_count = 1` group by aggregates suffer from 
the same problem



-- 
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] feat: support inability to yield for loop when it's not using Tok… [datafusion]

2025-05-28 Thread via GitHub 


zhuqi-lucas commented on PR #16196:
URL: https://github.com/apache/datafusion/pull/16196#issuecomment-2916000852

   Updated the performance for current PR:
   ```rust
   SET datafusion.execution.target_partitions = 1;
   
   SELECT SUM(value)
   FROM range(1,500) AS t;
   +--+
   | sum(t.value) |
   +--+
   | -4378597037249509888 |
   +--+
   1 row(s) fetched.
   Elapsed 22.315 seconds.
   ```
   
   
   ```rust
   SET datafusion.execution.target_partitions = 1;
   
   
   SELECT SUM(value)
   FROM range(1,500) AS t;
   +--+
   | sum(t.value) |
   +--+
   | -4378597037249509888 |
   +--+
   1 row(s) fetched.
   Elapsed 22.567 seconds.
   
   ```
   
   No performance regression from the above testing.
   
   


-- 
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]