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/08/09 02:43:57 UTC
[PR] Revive to use upstream arrow coalesce [datafusion]
zhuqi-lucas opened a new pull request, #17105:
URL: https://github.com/apache/datafusion/pull/17105
## Which issue does this PR close?
Revive https://github.com/apache/datafusion/pull/16249
## Rationale for this change
Revive https://github.com/apache/datafusion/pull/16249
And fix conflicts
## What changes are included in this PR?
This PR refactors the BatchCoalescer in DataFusion to use the proposed upstream API to show that it
Can be used (api is complete enough)
Is not any slower
## Are these changes tested?
Yes
## Are there any user-facing changes?
No
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3174436694
๐ค `./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-1016-gcp #16~24.04.1-Ubuntu SMP Wed May 28 02:40:52 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing revive_to_use_upstream_arrow_coalesce (8bbadaf7bfda68221d05c25cb2a4c527af2bebb7) to ab794d29b3783f379a5002753cf4b369086fb1fd [diff](https://github.com/apache/datafusion/compare/ab794d29b3783f379a5002753cf4b369086fb1fd..8bbadaf7bfda68221d05c25cb2a4c527af2bebb7) using: tpch_mem
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: 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: Revive to use upstream arrow coalesce [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#discussion_r2265100256
##########
datafusion/physical-plan/src/coalesce_batches.rs:
##########
@@ -277,102 +275,41 @@ impl Stream for CoalesceBatchesStream {
}
}
-/// Enumeration of possible states for `CoalesceBatchesStream`.
-/// It represents different stages in the lifecycle of a stream of record batches.
-///
-/// An example of state transition:
-/// Notation:
-/// `[3000]`: A batch with size 3000
-/// `{[2000], [3000]}`: `CoalesceBatchStream`'s internal buffer with 2 batches buffered
-/// Input of `CoalesceBatchStream` will generate three batches `[2000], [3000], [4000]`
-/// The coalescing procedure will go through the following steps with 4096 coalescing threshold:
-/// 1. Read the first batch and get it buffered.
-/// - initial state: `Pull`
-/// - initial buffer: `{}`
-/// - updated buffer: `{[2000]}`
-/// - next state: `Pull`
-/// 2. Read the second batch, the coalescing target is reached since 2000 + 3000 > 4096
-/// - initial state: `Pull`
-/// - initial buffer: `{[2000]}`
-/// - updated buffer: `{[2000], [3000]}`
-/// - next state: `ReturnBuffer`
-/// 4. Two batches in the batch get merged and consumed by the upstream operator.
-/// - initial state: `ReturnBuffer`
-/// - initial buffer: `{[2000], [3000]}`
-/// - updated buffer: `{}`
-/// - next state: `Pull`
-/// 5. Read the third input batch.
-/// - initial state: `Pull`
-/// - initial buffer: `{}`
-/// - updated buffer: `{[4000]}`
-/// - next state: `Pull`
-/// 5. The input is ended now. Jump to exhaustion state preparing the finalized data.
-/// - initial state: `Pull`
-/// - initial buffer: `{[4000]}`
-/// - updated buffer: `{[4000]}`
-/// - next state: `Exhausted`
-#[derive(Debug, Clone, Eq, PartialEq)]
-enum CoalesceBatchesStreamState {
- /// State to pull a new batch from the input stream.
- Pull,
- /// State to return a buffered batch.
- ReturnBuffer,
- /// State indicating that the stream is exhausted.
- Exhausted,
-}
-
impl CoalesceBatchesStream {
fn poll_next_inner(
self: &mut Pin<&mut Self>,
cx: &mut Context<'_>,
) -> Poll<Option<Result<RecordBatch>>> {
let cloned_time = self.baseline_metrics.elapsed_compute().clone();
loop {
- match &self.inner_state {
- CoalesceBatchesStreamState::Pull => {
- // Attempt to pull the next batch from the input stream.
- let input_batch = ready!(self.input.poll_next_unpin(cx));
- // Start timing the operation. The timer records time upon being dropped.
- let _timer = cloned_time.timer();
-
- match input_batch {
- Some(Ok(batch)) => match self.coalescer.push_batch(batch) {
- CoalescerState::Continue => {}
- CoalescerState::LimitReached => {
- self.inner_state = CoalesceBatchesStreamState::Exhausted;
- }
- CoalescerState::TargetReached => {
- self.inner_state =
- CoalesceBatchesStreamState::ReturnBuffer;
- }
- },
- None => {
- // End of input stream, but buffered batches might still be present.
- self.inner_state = CoalesceBatchesStreamState::Exhausted;
- }
- other => return Poll::Ready(other),
- }
- }
- CoalesceBatchesStreamState::ReturnBuffer => {
- let _timer = cloned_time.timer();
- // Combine buffered batches into one batch and return it.
- let batch = self.coalescer.finish_batch()?;
- // Set to pull state for the next iteration.
- self.inner_state = CoalesceBatchesStreamState::Pull;
- return Poll::Ready(Some(Ok(batch)));
+ // If there is any completed batch ready, return it
+ if let Some(batch) = self.coalescer.next_completed_batch() {
+ return Poll::Ready(Some(Ok(batch)));
+ }
+ if self.completed {
+ // If input is done and no batches are ready, return None to signal end of stream.
+ return Poll::Ready(None);
+ }
+ // Attempt to pull the next batch from the input stream.
+ let input_batch = ready!(self.input.poll_next_unpin(cx));
+ // Start timing the operation. The timer records time upon being dropped.
+ let _timer = cloned_time.timer();
+
+ match input_batch {
+ None => {
+ // Input stream is exhausted, finalize any remaining batches
+ self.completed = true;
+ self.coalescer.finish()?;
}
- CoalesceBatchesStreamState::Exhausted => {
- // Handle the end of the input stream.
- return if self.coalescer.is_empty() {
- // If buffer is empty, return None indicating the stream is fully consumed.
- Poll::Ready(None)
- } else {
- let _timer = cloned_time.timer();
- // If the buffer still contains batches, prepare to return them.
- let batch = self.coalescer.finish_batch()?;
- Poll::Ready(Some(Ok(batch)))
- };
+ Some(Ok(batch)) => {
+ if self.coalescer.push_batch(batch)? {
Review Comment:
Addressed it in latest PR, it returns an enum 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: Revive to use upstream arrow coalesce [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3170585484
๐ค: Benchmark completed
<details><summary>Details</summary>
<p>
```
Comparing HEAD and revive_to_use_upstream_arrow_coalesce
--------------------
Benchmark clickbench_extended.json
--------------------
โโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโ
โ Query โ HEAD โ revive_to_use_upstream_arrow_coalesce โ Change โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ QQuery 0 โ 1978.72 ms โ 1944.77 ms โ no change โ
โ QQuery 1 โ 771.03 ms โ 643.75 ms โ +1.20x faster โ
โ QQuery 2 โ 1504.59 ms โ 1283.66 ms โ +1.17x faster โ
โ QQuery 3 โ 618.98 ms โ 618.31 ms โ no change โ
โ QQuery 4 โ 1307.14 ms โ 1350.94 ms โ no change โ
โ QQuery 5 โ 13852.75 ms โ 13996.89 ms โ no change โ
โ QQuery 6 โ 2285.06 ms โ 2155.42 ms โ +1.06x faster โ
โ QQuery 7 โ 1795.16 ms โ 1819.77 ms โ no change โ
โโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโ
โ Benchmark Summary โ โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ Total Time (HEAD) โ 24113.42ms โ
โ Total Time (revive_to_use_upstream_arrow_coalesce) โ 23813.51ms โ
โ Average Time (HEAD) โ 3014.18ms โ
โ Average Time (revive_to_use_upstream_arrow_coalesce) โ 2976.69ms โ
โ Queries Faster โ 3 โ
โ Queries Slower โ 0 โ
โ Queries with No Change โ 5 โ
โ Queries with Failure โ 0 โ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโ
--------------------
Benchmark clickbench_partitioned.json
--------------------
โโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโ
โ Query โ HEAD โ revive_to_use_upstream_arrow_coalesce โ Change โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ QQuery 0 โ 2.35 ms โ 2.64 ms โ 1.12x slower โ
โ QQuery 1 โ 27.83 ms โ 29.15 ms โ no change โ
โ QQuery 2 โ 74.71 ms โ 73.36 ms โ no change โ
โ QQuery 3 โ 91.03 ms โ 89.84 ms โ no change โ
โ QQuery 4 โ 595.96 ms โ 602.12 ms โ no change โ
โ QQuery 5 โ 860.37 ms โ 835.65 ms โ no change โ
โ QQuery 6 โ 2.20 ms โ 2.20 ms โ no change โ
โ QQuery 7 โ 31.81 ms โ 32.72 ms โ no change โ
โ QQuery 8 โ 842.59 ms โ 846.27 ms โ no change โ
โ QQuery 9 โ 1160.15 ms โ 1133.52 ms โ no change โ
โ QQuery 10 โ 223.96 ms โ 213.82 ms โ no change โ
โ QQuery 11 โ 251.04 ms โ 242.97 ms โ no change โ
โ QQuery 12 โ 826.48 ms โ 815.01 ms โ no change โ
โ QQuery 13 โ 1193.58 ms โ 1188.55 ms โ no change โ
โ QQuery 14 โ 778.58 ms โ 771.01 ms โ no change โ
โ QQuery 15 โ 765.85 ms โ 772.44 ms โ no change โ
โ QQuery 16 โ 1576.56 ms โ 1578.18 ms โ no change โ
โ QQuery 17 โ 1580.58 ms โ 1578.56 ms โ no change โ
โ QQuery 18 โ 2820.92 ms โ 2842.70 ms โ no change โ
โ QQuery 19 โ 80.25 ms โ 79.64 ms โ no change โ
โ QQuery 20 โ 1152.98 ms โ 1100.78 ms โ no change โ
โ QQuery 21 โ 1302.14 ms โ 1259.29 ms โ no change โ
โ QQuery 22 โ 2136.74 ms โ 2064.58 ms โ no change โ
โ QQuery 23 โ 7499.03 ms โ 7311.46 ms โ no change โ
โ QQuery 24 โ 395.37 ms โ 376.85 ms โ no change โ
โ QQuery 25 โ 274.14 ms โ 254.31 ms โ +1.08x faster โ
โ QQuery 26 โ 395.58 ms โ 375.71 ms โ +1.05x faster โ
โ QQuery 27 โ 1519.29 ms โ 1518.21 ms โ no change โ
โ QQuery 28 โ 11788.81 ms โ 12181.37 ms โ no change โ
โ QQuery 29 โ 526.17 ms โ 513.72 ms โ no change โ
โ QQuery 30 โ 753.70 ms โ 744.32 ms โ no change โ
โ QQuery 31 โ 768.23 ms โ 769.84 ms โ no change โ
โ QQuery 32 โ 2363.83 ms โ 2389.26 ms โ no change โ
โ QQuery 33 โ 3147.88 ms โ 3116.51 ms โ no change โ
โ QQuery 34 โ 3196.56 ms โ 3154.66 ms โ no change โ
โ QQuery 35 โ 1213.64 ms โ 1200.73 ms โ no change โ
โ QQuery 36 โ 123.40 ms โ 124.99 ms โ no change โ
โ QQuery 37 โ 54.61 ms โ 50.30 ms โ +1.09x faster โ
โ QQuery 38 โ 121.09 ms โ 118.44 ms โ no change โ
โ QQuery 39 โ 198.96 ms โ 192.82 ms โ no change โ
โ QQuery 40 โ 41.06 ms โ 40.60 ms โ no change โ
โ QQuery 41 โ 40.71 ms โ 40.11 ms โ no change โ
โ QQuery 42 โ 35.87 ms โ 36.53 ms โ no change โ
โโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโ
โ Benchmark Summary โ โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ Total Time (HEAD) โ 52836.59ms โ
โ Total Time (revive_to_use_upstream_arrow_coalesce) โ 52665.74ms โ
โ Average Time (HEAD) โ 1228.76ms โ
โ Average Time (revive_to_use_upstream_arrow_coalesce) โ 1224.78ms โ
โ Queries Faster โ 3 โ
โ Queries Slower โ 1 โ
โ Queries with No Change โ 39 โ
โ Queries with Failure โ 0 โ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโ
--------------------
Benchmark tpch_mem_sf1.json
--------------------
โโโโโโโโโโโโโโโโณโโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโ
โ Query โ HEAD โ revive_to_use_upstream_arrow_coalesce โ Change โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ QQuery 1 โ 94.69 ms โ 93.93 ms โ no change โ
โ QQuery 2 โ 20.98 ms โ 22.54 ms โ 1.07x slower โ
โ QQuery 3 โ 32.02 ms โ 37.29 ms โ 1.16x slower โ
โ QQuery 4 โ 18.45 ms โ 20.03 ms โ 1.09x slower โ
โ QQuery 5 โ 48.49 ms โ 54.68 ms โ 1.13x slower โ
โ QQuery 6 โ 11.94 ms โ 11.63 ms โ no change โ
โ QQuery 7 โ 85.30 ms โ 100.53 ms โ 1.18x slower โ
โ QQuery 8 โ 26.25 ms โ 28.29 ms โ 1.08x slower โ
โ QQuery 9 โ 55.43 ms โ 58.77 ms โ 1.06x slower โ
โ QQuery 10 โ 39.79 ms โ 43.39 ms โ 1.09x slower โ
โ QQuery 11 โ 10.97 ms โ 11.70 ms โ 1.07x slower โ
โ QQuery 12 โ 29.20 ms โ 30.29 ms โ no change โ
โ QQuery 13 โ 25.57 ms โ 26.68 ms โ no change โ
โ QQuery 14 โ 9.50 ms โ 10.18 ms โ 1.07x slower โ
โ QQuery 15 โ 18.37 ms โ 18.72 ms โ no change โ
โ QQuery 16 โ 17.21 ms โ 18.79 ms โ 1.09x slower โ
โ QQuery 17 โ 94.62 ms โ 98.44 ms โ no change โ
โ QQuery 18 โ 175.22 ms โ 174.39 ms โ no change โ
โ QQuery 19 โ 23.86 ms โ 25.42 ms โ 1.07x slower โ
โ QQuery 20 โ 30.88 ms โ 33.14 ms โ 1.07x slower โ
โ QQuery 21 โ 140.56 ms โ 157.40 ms โ 1.12x slower โ
โ QQuery 22 โ 15.25 ms โ 16.53 ms โ 1.08x slower โ
โโโโโโโโโโโโโโโโดโโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโ
โ Benchmark Summary โ โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ Total Time (HEAD) โ 1024.57ms โ
โ Total Time (revive_to_use_upstream_arrow_coalesce) โ 1092.74ms โ
โ Average Time (HEAD) โ 46.57ms โ
โ Average Time (revive_to_use_upstream_arrow_coalesce) โ 49.67ms โ
โ Queries Faster โ 0 โ
โ Queries Slower โ 15 โ
โ Queries with No Change โ 7 โ
โ Queries with Failure โ 0 โ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโ
```
</p>
</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: 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: Revive to use upstream arrow coalesce [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3169768311
FYI @alamb @Dandandan
I try to revive the PR https://github.com/apache/datafusion/pull/16249, and we may run the benchmark for this PR to see if any changes since then, 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: Revive to use upstream arrow coalesce [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3174434984
> The clickbench result is good, but tpch_mem seems some regression from the benchmark result. ๐ค
Weird, I will rerun and see if we can see it again
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3174858830
> > ๐ค the new kernel seems to slow down. I wonder if the overhead of precisely sized output batches is causing the issue
>
> Good point @alamb , i agree this is the only difference. I can add a test PR to make upstream do not generate precisely sized output batches, but when we ensure capacity for the increment buffer size, it seems we need to make the size change since we do not keep the same target size for this change.
>
> The latest benchmark seems a little better.
> > ```
Thanks @zhuqi-lucas -- what I was thinking about was something like the following
```rust
let target_batch_size = 4;
let mut coalescer = BatchCoalescer::new(batch1.schema(), 4)
.with_exact_size(false)
```
Before we spend a lot of time polishing / testing a PR for that it would probably be good to hack up a POC and verify it actually improves performance
Thank you for your willingness to help along with this project. It is something I have thought was important (but not critical) for a long time and so having someone to help really makes a big difference
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3170558361
I was just thinking about this PR last night -- thank you @zhuqi-lucas -- I'll kick off the benchmarks just to make sure
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3174529560
๐ค I wonder if the overhead of precisely sized output batches is causing the issue ๐ค
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3174569793
> ๐ค the new kernel seems to slow down. I wonder if the overhead of precisely sized output batches is causing the issue
Good point @alamb , i agree this is the only difference. I can add a test PR to make upstream do not generate precisely sized output batches, but when we ensure capacity, it seems we need to make the size change since we do not keep the same target size for this change.
The latest benchmark seems a little better.
> ๐ค: Benchmark completed
>
> Details
>
> ```
> Comparing HEAD and revive_to_use_upstream_arrow_coalesce
> --------------------
> Benchmark tpch_mem_sf1.json
> --------------------
> โโโโโโโโโโโโโโโโณโโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโ
> โ Query โ HEAD โ revive_to_use_upstream_arrow_coalesce โ Change โ
> โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
> โ QQuery 1 โ 95.61 ms โ 92.56 ms โ no change โ
> โ QQuery 2 โ 20.85 ms โ 22.44 ms โ 1.08x slower โ
> โ QQuery 3 โ 31.93 ms โ 36.28 ms โ 1.14x slower โ
> โ QQuery 4 โ 18.15 ms โ 19.48 ms โ 1.07x slower โ
> โ QQuery 5 โ 48.85 ms โ 53.88 ms โ 1.10x slower โ
> โ QQuery 6 โ 12.03 ms โ 11.62 ms โ no change โ
> โ QQuery 7 โ 88.27 ms โ 93.17 ms โ 1.06x slower โ
> โ QQuery 8 โ 24.33 ms โ 24.10 ms โ no change โ
> โ QQuery 9 โ 53.84 ms โ 55.02 ms โ no change โ
> โ QQuery 10 โ 40.33 ms โ 41.29 ms โ no change โ
> โ QQuery 11 โ 11.20 ms โ 11.65 ms โ no change โ
> โ QQuery 12 โ 29.78 ms โ 29.46 ms โ no change โ
> โ QQuery 13 โ 25.89 ms โ 26.05 ms โ no change โ
> โ QQuery 14 โ 9.66 ms โ 10.02 ms โ no change โ
> โ QQuery 15 โ 19.14 ms โ 18.82 ms โ no change โ
> โ QQuery 16 โ 17.42 ms โ 17.74 ms โ no change โ
> โ QQuery 17 โ 96.53 ms โ 95.86 ms โ no change โ
> โ QQuery 18 โ 179.04 ms โ 179.63 ms โ no change โ
> โ QQuery 19 โ 24.01 ms โ 26.11 ms โ 1.09x slower โ
> โ QQuery 20 โ 31.57 ms โ 33.97 ms โ 1.08x slower โ
> โ QQuery 21 โ 139.56 ms โ 151.41 ms โ 1.08x slower โ
> โ QQuery 22 โ 13.78 ms โ 14.77 ms โ 1.07x slower โ
> โโโโโโโโโโโโโโโโดโโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโ
> โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโ
> โ Benchmark Summary โ โ
> โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
> โ Total Time (HEAD) โ 1031.76ms โ
> โ Total Time (revive_to_use_upstream_arrow_coalesce) โ 1065.32ms โ
> โ Average Time (HEAD) โ 46.90ms โ
> โ Average Time (revive_to_use_upstream_arrow_coalesce) โ 48.42ms โ
> โ Queries Faster โ 0 โ
> โ Queries Slower โ 9 โ
> โ Queries with No Change โ 13 โ
> โ Queries with Failure โ 0 โ
> โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโ
> ```
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3182704380
For the `tpch_mem` slowdown, another possible reason could be unnecessary copies for batches that are exactly `batch_size`.
For certain operators, there might already be an internal mechanism to ensure their output is exactly batch_size. From a quick look at the implementation, the old version could pass such batches through directly, whereas this PR forces them to be copied.
Another potential improvement: could we make this pass-through threshold more lenient? For example, if the coalescer receives a batch with size >= `batch_size / 2`, it could pass it through without coalescing. In such cases, the output size is already large enough to benefit from vectorization, so the extra concatenation might not add much value.
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3188162121
Man this one is tricky -- like many performance optimization quests, it will likely take careful engineering and investigation. It is fascinating to me that what in theory should be a net neutral change is turning out to be slower in some cases
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3188036331
> ๐ค: Benchmark completed
>
> Details
>
> ```
> Comparing HEAD and revive_to_use_upstream_arrow_coalesce
> --------------------
> Benchmark tpch_mem_sf1.json
> --------------------
> โโโโโโโโโโโโโโโโณโโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโ
> โ Query โ HEAD โ revive_to_use_upstream_arrow_coalesce โ Change โ
> โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
> โ QQuery 1 โ 97.06 ms โ 83.14 ms โ +1.17x faster โ
> โ QQuery 2 โ 20.75 ms โ 23.00 ms โ 1.11x slower โ
> โ QQuery 3 โ 32.13 ms โ 35.53 ms โ 1.11x slower โ
> โ QQuery 4 โ 18.36 ms โ 19.71 ms โ 1.07x slower โ
> โ QQuery 5 โ 49.02 ms โ 55.08 ms โ 1.12x slower โ
> โ QQuery 6 โ 11.81 ms โ 11.65 ms โ no change โ
> โ QQuery 7 โ 89.03 ms โ 94.83 ms โ 1.07x slower โ
> โ QQuery 8 โ 23.60 ms โ 25.81 ms โ 1.09x slower โ
> โ QQuery 9 โ 52.99 ms โ 56.22 ms โ 1.06x slower โ
> โ QQuery 10 โ 40.33 ms โ 41.90 ms โ no change โ
> โ QQuery 11 โ 11.12 ms โ 11.81 ms โ 1.06x slower โ
> โ QQuery 12 โ 30.00 ms โ 30.29 ms โ no change โ
> โ QQuery 13 โ 25.83 ms โ 25.76 ms โ no change โ
> โ QQuery 14 โ 9.92 ms โ 10.32 ms โ no change โ
> โ QQuery 15 โ 18.48 ms โ 19.27 ms โ no change โ
> โ QQuery 16 โ 17.50 ms โ 17.95 ms โ no change โ
> โ QQuery 17 โ 97.90 ms โ 99.31 ms โ no change โ
> โ QQuery 18 โ 186.46 ms โ 173.42 ms โ +1.08x faster โ
> โ QQuery 19 โ 24.07 ms โ 25.47 ms โ 1.06x slower โ
> โ QQuery 20 โ 31.98 ms โ 33.17 ms โ no change โ
> โ QQuery 21 โ 142.18 ms โ 151.87 ms โ 1.07x slower โ
> โ QQuery 22 โ 14.83 ms โ 14.71 ms โ no change โ
> โโโโโโโโโโโโโโโโดโโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโ
> โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโ
> โ Benchmark Summary โ โ
> โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
> โ Total Time (HEAD) โ 1045.35ms โ
> โ Total Time (revive_to_use_upstream_arrow_coalesce) โ 1060.21ms โ
> โ Average Time (HEAD) โ 47.52ms โ
> โ Average Time (revive_to_use_upstream_arrow_coalesce) โ 48.19ms โ
> โ Queries Faster โ 2 โ
> โ Queries Slower โ 10 โ
> โ Queries with No Change โ 10 โ
> โ Queries with Failure โ 0 โ
> โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโ
> ```
cc @alamb @2010YOUY01 It seems still regression after addressing the comments. I need more investigation and try.
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3170564243
> Thank you @zhuqi-lucas -- this looks great
>
> As long as the benchmarks look good (I expect no substantial change) I think we should merge this
Thank you @alamb for review, my follow-up plan, correct me if i am wrong:
1. I will try to address this comments from @Dandandan https://github.com/apache/datafusion/pull/16249#issuecomment-2974721937 for this PR or follow-up to improve performance.
2. Optimize the push_batch_with_filter performance, i can start from primitive type:
https://github.com/apache/arrow-rs/issues/7762
3. I can investigate try to change filter_exec with coalesce_batch_exec to coalesce_batch_exec(with filter) which can use upstream push_batch_with_filter, we may can remove coalesce_batch_exec operator in future?
4. Apply to other combination with coalesce_batch_exec, such HashJoinExec, RepartitionExec, etc.
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3185865604
๐ค: Benchmark completed
<details><summary>Details</summary>
<p>
```
Comparing HEAD and revive_to_use_upstream_arrow_coalesce
--------------------
Benchmark tpch_mem_sf1.json
--------------------
โโโโโโโโโโโโโโโโณโโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโ
โ Query โ HEAD โ revive_to_use_upstream_arrow_coalesce โ Change โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ QQuery 1 โ 97.06 ms โ 83.14 ms โ +1.17x faster โ
โ QQuery 2 โ 20.75 ms โ 23.00 ms โ 1.11x slower โ
โ QQuery 3 โ 32.13 ms โ 35.53 ms โ 1.11x slower โ
โ QQuery 4 โ 18.36 ms โ 19.71 ms โ 1.07x slower โ
โ QQuery 5 โ 49.02 ms โ 55.08 ms โ 1.12x slower โ
โ QQuery 6 โ 11.81 ms โ 11.65 ms โ no change โ
โ QQuery 7 โ 89.03 ms โ 94.83 ms โ 1.07x slower โ
โ QQuery 8 โ 23.60 ms โ 25.81 ms โ 1.09x slower โ
โ QQuery 9 โ 52.99 ms โ 56.22 ms โ 1.06x slower โ
โ QQuery 10 โ 40.33 ms โ 41.90 ms โ no change โ
โ QQuery 11 โ 11.12 ms โ 11.81 ms โ 1.06x slower โ
โ QQuery 12 โ 30.00 ms โ 30.29 ms โ no change โ
โ QQuery 13 โ 25.83 ms โ 25.76 ms โ no change โ
โ QQuery 14 โ 9.92 ms โ 10.32 ms โ no change โ
โ QQuery 15 โ 18.48 ms โ 19.27 ms โ no change โ
โ QQuery 16 โ 17.50 ms โ 17.95 ms โ no change โ
โ QQuery 17 โ 97.90 ms โ 99.31 ms โ no change โ
โ QQuery 18 โ 186.46 ms โ 173.42 ms โ +1.08x faster โ
โ QQuery 19 โ 24.07 ms โ 25.47 ms โ 1.06x slower โ
โ QQuery 20 โ 31.98 ms โ 33.17 ms โ no change โ
โ QQuery 21 โ 142.18 ms โ 151.87 ms โ 1.07x slower โ
โ QQuery 22 โ 14.83 ms โ 14.71 ms โ no change โ
โโโโโโโโโโโโโโโโดโโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโ
โ Benchmark Summary โ โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ Total Time (HEAD) โ 1045.35ms โ
โ Total Time (revive_to_use_upstream_arrow_coalesce) โ 1060.21ms โ
โ Average Time (HEAD) โ 47.52ms โ
โ Average Time (revive_to_use_upstream_arrow_coalesce) โ 48.19ms โ
โ Queries Faster โ 2 โ
โ Queries Slower โ 10 โ
โ Queries with No Change โ 10 โ
โ Queries with Failure โ 0 โ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโ
```
</p>
</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: 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: Revive to use upstream arrow coalesce [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed pull request #17105: Feat: Revive to use upstream arrow coalesce
URL: https://github.com/apache/datafusion/pull/17105
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#discussion_r2264640220
##########
datafusion/physical-plan/src/coalesce_batches.rs:
##########
@@ -277,102 +275,41 @@ impl Stream for CoalesceBatchesStream {
}
}
-/// Enumeration of possible states for `CoalesceBatchesStream`.
-/// It represents different stages in the lifecycle of a stream of record batches.
-///
-/// An example of state transition:
-/// Notation:
-/// `[3000]`: A batch with size 3000
-/// `{[2000], [3000]}`: `CoalesceBatchStream`'s internal buffer with 2 batches buffered
-/// Input of `CoalesceBatchStream` will generate three batches `[2000], [3000], [4000]`
-/// The coalescing procedure will go through the following steps with 4096 coalescing threshold:
-/// 1. Read the first batch and get it buffered.
-/// - initial state: `Pull`
-/// - initial buffer: `{}`
-/// - updated buffer: `{[2000]}`
-/// - next state: `Pull`
-/// 2. Read the second batch, the coalescing target is reached since 2000 + 3000 > 4096
-/// - initial state: `Pull`
-/// - initial buffer: `{[2000]}`
-/// - updated buffer: `{[2000], [3000]}`
-/// - next state: `ReturnBuffer`
-/// 4. Two batches in the batch get merged and consumed by the upstream operator.
-/// - initial state: `ReturnBuffer`
-/// - initial buffer: `{[2000], [3000]}`
-/// - updated buffer: `{}`
-/// - next state: `Pull`
-/// 5. Read the third input batch.
-/// - initial state: `Pull`
-/// - initial buffer: `{}`
-/// - updated buffer: `{[4000]}`
-/// - next state: `Pull`
-/// 5. The input is ended now. Jump to exhaustion state preparing the finalized data.
-/// - initial state: `Pull`
-/// - initial buffer: `{[4000]}`
-/// - updated buffer: `{[4000]}`
-/// - next state: `Exhausted`
-#[derive(Debug, Clone, Eq, PartialEq)]
-enum CoalesceBatchesStreamState {
- /// State to pull a new batch from the input stream.
- Pull,
- /// State to return a buffered batch.
- ReturnBuffer,
- /// State indicating that the stream is exhausted.
- Exhausted,
-}
-
impl CoalesceBatchesStream {
fn poll_next_inner(
self: &mut Pin<&mut Self>,
cx: &mut Context<'_>,
) -> Poll<Option<Result<RecordBatch>>> {
let cloned_time = self.baseline_metrics.elapsed_compute().clone();
loop {
- match &self.inner_state {
- CoalesceBatchesStreamState::Pull => {
- // Attempt to pull the next batch from the input stream.
- let input_batch = ready!(self.input.poll_next_unpin(cx));
- // Start timing the operation. The timer records time upon being dropped.
- let _timer = cloned_time.timer();
-
- match input_batch {
- Some(Ok(batch)) => match self.coalescer.push_batch(batch) {
- CoalescerState::Continue => {}
- CoalescerState::LimitReached => {
- self.inner_state = CoalesceBatchesStreamState::Exhausted;
- }
- CoalescerState::TargetReached => {
- self.inner_state =
- CoalesceBatchesStreamState::ReturnBuffer;
- }
- },
- None => {
- // End of input stream, but buffered batches might still be present.
- self.inner_state = CoalesceBatchesStreamState::Exhausted;
- }
- other => return Poll::Ready(other),
- }
- }
- CoalesceBatchesStreamState::ReturnBuffer => {
- let _timer = cloned_time.timer();
- // Combine buffered batches into one batch and return it.
- let batch = self.coalescer.finish_batch()?;
- // Set to pull state for the next iteration.
- self.inner_state = CoalesceBatchesStreamState::Pull;
- return Poll::Ready(Some(Ok(batch)));
+ // If there is any completed batch ready, return it
+ if let Some(batch) = self.coalescer.next_completed_batch() {
+ return Poll::Ready(Some(Ok(batch)));
+ }
+ if self.completed {
+ // If input is done and no batches are ready, return None to signal end of stream.
+ return Poll::Ready(None);
+ }
+ // Attempt to pull the next batch from the input stream.
+ let input_batch = ready!(self.input.poll_next_unpin(cx));
+ // Start timing the operation. The timer records time upon being dropped.
+ let _timer = cloned_time.timer();
+
+ match input_batch {
+ None => {
+ // Input stream is exhausted, finalize any remaining batches
+ self.completed = true;
+ self.coalescer.finish()?;
}
- CoalesceBatchesStreamState::Exhausted => {
- // Handle the end of the input stream.
- return if self.coalescer.is_empty() {
- // If buffer is empty, return None indicating the stream is fully consumed.
- Poll::Ready(None)
- } else {
- let _timer = cloned_time.timer();
- // If the buffer still contains batches, prepare to return them.
- let batch = self.coalescer.finish_batch()?;
- Poll::Ready(Some(Ok(batch)))
- };
+ Some(Ok(batch)) => {
+ if self.coalescer.push_batch(batch)? {
Review Comment:
Thank you @alamb for good suggestion, this is a better way i agree.
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3170598824
> ๐ค: Benchmark completed
>
> Details
>
> ```
> Comparing HEAD and revive_to_use_upstream_arrow_coalesce
> --------------------
> Benchmark clickbench_extended.json
> --------------------
> โโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโ
> โ Query โ HEAD โ revive_to_use_upstream_arrow_coalesce โ Change โ
> โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
> โ QQuery 0 โ 1978.72 ms โ 1944.77 ms โ no change โ
> โ QQuery 1 โ 771.03 ms โ 643.75 ms โ +1.20x faster โ
> โ QQuery 2 โ 1504.59 ms โ 1283.66 ms โ +1.17x faster โ
> โ QQuery 3 โ 618.98 ms โ 618.31 ms โ no change โ
> โ QQuery 4 โ 1307.14 ms โ 1350.94 ms โ no change โ
> โ QQuery 5 โ 13852.75 ms โ 13996.89 ms โ no change โ
> โ QQuery 6 โ 2285.06 ms โ 2155.42 ms โ +1.06x faster โ
> โ QQuery 7 โ 1795.16 ms โ 1819.77 ms โ no change โ
> โโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโ
> โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโ
> โ Benchmark Summary โ โ
> โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
> โ Total Time (HEAD) โ 24113.42ms โ
> โ Total Time (revive_to_use_upstream_arrow_coalesce) โ 23813.51ms โ
> โ Average Time (HEAD) โ 3014.18ms โ
> โ Average Time (revive_to_use_upstream_arrow_coalesce) โ 2976.69ms โ
> โ Queries Faster โ 3 โ
> โ Queries Slower โ 0 โ
> โ Queries with No Change โ 5 โ
> โ Queries with Failure โ 0 โ
> โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโ
> --------------------
> Benchmark clickbench_partitioned.json
> --------------------
> โโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโ
> โ Query โ HEAD โ revive_to_use_upstream_arrow_coalesce โ Change โ
> โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
> โ QQuery 0 โ 2.35 ms โ 2.64 ms โ 1.12x slower โ
> โ QQuery 1 โ 27.83 ms โ 29.15 ms โ no change โ
> โ QQuery 2 โ 74.71 ms โ 73.36 ms โ no change โ
> โ QQuery 3 โ 91.03 ms โ 89.84 ms โ no change โ
> โ QQuery 4 โ 595.96 ms โ 602.12 ms โ no change โ
> โ QQuery 5 โ 860.37 ms โ 835.65 ms โ no change โ
> โ QQuery 6 โ 2.20 ms โ 2.20 ms โ no change โ
> โ QQuery 7 โ 31.81 ms โ 32.72 ms โ no change โ
> โ QQuery 8 โ 842.59 ms โ 846.27 ms โ no change โ
> โ QQuery 9 โ 1160.15 ms โ 1133.52 ms โ no change โ
> โ QQuery 10 โ 223.96 ms โ 213.82 ms โ no change โ
> โ QQuery 11 โ 251.04 ms โ 242.97 ms โ no change โ
> โ QQuery 12 โ 826.48 ms โ 815.01 ms โ no change โ
> โ QQuery 13 โ 1193.58 ms โ 1188.55 ms โ no change โ
> โ QQuery 14 โ 778.58 ms โ 771.01 ms โ no change โ
> โ QQuery 15 โ 765.85 ms โ 772.44 ms โ no change โ
> โ QQuery 16 โ 1576.56 ms โ 1578.18 ms โ no change โ
> โ QQuery 17 โ 1580.58 ms โ 1578.56 ms โ no change โ
> โ QQuery 18 โ 2820.92 ms โ 2842.70 ms โ no change โ
> โ QQuery 19 โ 80.25 ms โ 79.64 ms โ no change โ
> โ QQuery 20 โ 1152.98 ms โ 1100.78 ms โ no change โ
> โ QQuery 21 โ 1302.14 ms โ 1259.29 ms โ no change โ
> โ QQuery 22 โ 2136.74 ms โ 2064.58 ms โ no change โ
> โ QQuery 23 โ 7499.03 ms โ 7311.46 ms โ no change โ
> โ QQuery 24 โ 395.37 ms โ 376.85 ms โ no change โ
> โ QQuery 25 โ 274.14 ms โ 254.31 ms โ +1.08x faster โ
> โ QQuery 26 โ 395.58 ms โ 375.71 ms โ +1.05x faster โ
> โ QQuery 27 โ 1519.29 ms โ 1518.21 ms โ no change โ
> โ QQuery 28 โ 11788.81 ms โ 12181.37 ms โ no change โ
> โ QQuery 29 โ 526.17 ms โ 513.72 ms โ no change โ
> โ QQuery 30 โ 753.70 ms โ 744.32 ms โ no change โ
> โ QQuery 31 โ 768.23 ms โ 769.84 ms โ no change โ
> โ QQuery 32 โ 2363.83 ms โ 2389.26 ms โ no change โ
> โ QQuery 33 โ 3147.88 ms โ 3116.51 ms โ no change โ
> โ QQuery 34 โ 3196.56 ms โ 3154.66 ms โ no change โ
> โ QQuery 35 โ 1213.64 ms โ 1200.73 ms โ no change โ
> โ QQuery 36 โ 123.40 ms โ 124.99 ms โ no change โ
> โ QQuery 37 โ 54.61 ms โ 50.30 ms โ +1.09x faster โ
> โ QQuery 38 โ 121.09 ms โ 118.44 ms โ no change โ
> โ QQuery 39 โ 198.96 ms โ 192.82 ms โ no change โ
> โ QQuery 40 โ 41.06 ms โ 40.60 ms โ no change โ
> โ QQuery 41 โ 40.71 ms โ 40.11 ms โ no change โ
> โ QQuery 42 โ 35.87 ms โ 36.53 ms โ no change โ
> โโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโ
> โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโ
> โ Benchmark Summary โ โ
> โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
> โ Total Time (HEAD) โ 52836.59ms โ
> โ Total Time (revive_to_use_upstream_arrow_coalesce) โ 52665.74ms โ
> โ Average Time (HEAD) โ 1228.76ms โ
> โ Average Time (revive_to_use_upstream_arrow_coalesce) โ 1224.78ms โ
> โ Queries Faster โ 3 โ
> โ Queries Slower โ 1 โ
> โ Queries with No Change โ 39 โ
> โ Queries with Failure โ 0 โ
> โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโ
> --------------------
> Benchmark tpch_mem_sf1.json
> --------------------
> โโโโโโโโโโโโโโโโณโโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโ
> โ Query โ HEAD โ revive_to_use_upstream_arrow_coalesce โ Change โ
> โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
> โ QQuery 1 โ 94.69 ms โ 93.93 ms โ no change โ
> โ QQuery 2 โ 20.98 ms โ 22.54 ms โ 1.07x slower โ
> โ QQuery 3 โ 32.02 ms โ 37.29 ms โ 1.16x slower โ
> โ QQuery 4 โ 18.45 ms โ 20.03 ms โ 1.09x slower โ
> โ QQuery 5 โ 48.49 ms โ 54.68 ms โ 1.13x slower โ
> โ QQuery 6 โ 11.94 ms โ 11.63 ms โ no change โ
> โ QQuery 7 โ 85.30 ms โ 100.53 ms โ 1.18x slower โ
> โ QQuery 8 โ 26.25 ms โ 28.29 ms โ 1.08x slower โ
> โ QQuery 9 โ 55.43 ms โ 58.77 ms โ 1.06x slower โ
> โ QQuery 10 โ 39.79 ms โ 43.39 ms โ 1.09x slower โ
> โ QQuery 11 โ 10.97 ms โ 11.70 ms โ 1.07x slower โ
> โ QQuery 12 โ 29.20 ms โ 30.29 ms โ no change โ
> โ QQuery 13 โ 25.57 ms โ 26.68 ms โ no change โ
> โ QQuery 14 โ 9.50 ms โ 10.18 ms โ 1.07x slower โ
> โ QQuery 15 โ 18.37 ms โ 18.72 ms โ no change โ
> โ QQuery 16 โ 17.21 ms โ 18.79 ms โ 1.09x slower โ
> โ QQuery 17 โ 94.62 ms โ 98.44 ms โ no change โ
> โ QQuery 18 โ 175.22 ms โ 174.39 ms โ no change โ
> โ QQuery 19 โ 23.86 ms โ 25.42 ms โ 1.07x slower โ
> โ QQuery 20 โ 30.88 ms โ 33.14 ms โ 1.07x slower โ
> โ QQuery 21 โ 140.56 ms โ 157.40 ms โ 1.12x slower โ
> โ QQuery 22 โ 15.25 ms โ 16.53 ms โ 1.08x slower โ
> โโโโโโโโโโโโโโโโดโโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโ
> โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโ
> โ Benchmark Summary โ โ
> โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
> โ Total Time (HEAD) โ 1024.57ms โ
> โ Total Time (revive_to_use_upstream_arrow_coalesce) โ 1092.74ms โ
> โ Average Time (HEAD) โ 46.57ms โ
> โ Average Time (revive_to_use_upstream_arrow_coalesce) โ 49.67ms โ
> โ Queries Faster โ 0 โ
> โ Queries Slower โ 15 โ
> โ Queries with No Change โ 7 โ
> โ Queries with Failure โ 0 โ
> โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโ
> ```
The clickbench result is good, but tpch_mem seems some regression from the benchmark 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: Revive to use upstream arrow coalesce [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3185731563
๐ค `./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-1016-gcp #16~24.04.1-Ubuntu SMP Wed May 28 02:40:52 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing revive_to_use_upstream_arrow_coalesce (4e31d7bcb0b2c40e8aac556fbcc128a32374099c) to 0b7186ebaf01ce7dc0e6eacc026bd48b0f534be6 [diff](https://github.com/apache/datafusion/compare/0b7186ebaf01ce7dc0e6eacc026bd48b0f534be6..4e31d7bcb0b2c40e8aac556fbcc128a32374099c) using: tpch_mem
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: 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: Revive to use upstream arrow coalesce [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#discussion_r2274591575
##########
datafusion/physical-plan/src/coalesce/mod.rs:
##########
@@ -113,9 +117,16 @@ impl LimitedBatchCoalescer {
}
}
+ let num_rows = batch.num_rows();
// Limit not reached, push the entire batch
self.total_rows += batch.num_rows();
self.inner.push_batch(batch)?;
+
+ // If the number of rows in the current batch exceeds the coalesce size,
Review Comment:
if this turns out to work well, maybe it would be a good heuristic to add to the underlying coalescer too in non strict mode ๐ค
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#discussion_r2264639473
##########
datafusion/physical-plan/src/coalesce_batches.rs:
##########
@@ -277,102 +275,41 @@ impl Stream for CoalesceBatchesStream {
}
}
-/// Enumeration of possible states for `CoalesceBatchesStream`.
-/// It represents different stages in the lifecycle of a stream of record batches.
-///
-/// An example of state transition:
-/// Notation:
-/// `[3000]`: A batch with size 3000
-/// `{[2000], [3000]}`: `CoalesceBatchStream`'s internal buffer with 2 batches buffered
-/// Input of `CoalesceBatchStream` will generate three batches `[2000], [3000], [4000]`
-/// The coalescing procedure will go through the following steps with 4096 coalescing threshold:
-/// 1. Read the first batch and get it buffered.
-/// - initial state: `Pull`
-/// - initial buffer: `{}`
-/// - updated buffer: `{[2000]}`
-/// - next state: `Pull`
-/// 2. Read the second batch, the coalescing target is reached since 2000 + 3000 > 4096
-/// - initial state: `Pull`
-/// - initial buffer: `{[2000]}`
-/// - updated buffer: `{[2000], [3000]}`
-/// - next state: `ReturnBuffer`
-/// 4. Two batches in the batch get merged and consumed by the upstream operator.
-/// - initial state: `ReturnBuffer`
-/// - initial buffer: `{[2000], [3000]}`
-/// - updated buffer: `{}`
-/// - next state: `Pull`
-/// 5. Read the third input batch.
-/// - initial state: `Pull`
-/// - initial buffer: `{}`
-/// - updated buffer: `{[4000]}`
-/// - next state: `Pull`
-/// 5. The input is ended now. Jump to exhaustion state preparing the finalized data.
-/// - initial state: `Pull`
-/// - initial buffer: `{[4000]}`
-/// - updated buffer: `{[4000]}`
-/// - next state: `Exhausted`
-#[derive(Debug, Clone, Eq, PartialEq)]
-enum CoalesceBatchesStreamState {
- /// State to pull a new batch from the input stream.
- Pull,
- /// State to return a buffered batch.
- ReturnBuffer,
- /// State indicating that the stream is exhausted.
- Exhausted,
-}
-
impl CoalesceBatchesStream {
fn poll_next_inner(
self: &mut Pin<&mut Self>,
cx: &mut Context<'_>,
) -> Poll<Option<Result<RecordBatch>>> {
let cloned_time = self.baseline_metrics.elapsed_compute().clone();
loop {
- match &self.inner_state {
- CoalesceBatchesStreamState::Pull => {
- // Attempt to pull the next batch from the input stream.
- let input_batch = ready!(self.input.poll_next_unpin(cx));
- // Start timing the operation. The timer records time upon being dropped.
- let _timer = cloned_time.timer();
-
- match input_batch {
- Some(Ok(batch)) => match self.coalescer.push_batch(batch) {
- CoalescerState::Continue => {}
- CoalescerState::LimitReached => {
- self.inner_state = CoalesceBatchesStreamState::Exhausted;
- }
- CoalescerState::TargetReached => {
- self.inner_state =
- CoalesceBatchesStreamState::ReturnBuffer;
- }
- },
- None => {
- // End of input stream, but buffered batches might still be present.
- self.inner_state = CoalesceBatchesStreamState::Exhausted;
- }
- other => return Poll::Ready(other),
- }
- }
- CoalesceBatchesStreamState::ReturnBuffer => {
- let _timer = cloned_time.timer();
- // Combine buffered batches into one batch and return it.
- let batch = self.coalescer.finish_batch()?;
- // Set to pull state for the next iteration.
- self.inner_state = CoalesceBatchesStreamState::Pull;
- return Poll::Ready(Some(Ok(batch)));
+ // If there is any completed batch ready, return it
+ if let Some(batch) = self.coalescer.next_completed_batch() {
+ return Poll::Ready(Some(Ok(batch)));
+ }
+ if self.completed {
+ // If input is done and no batches are ready, return None to signal end of stream.
+ return Poll::Ready(None);
+ }
+ // Attempt to pull the next batch from the input stream.
+ let input_batch = ready!(self.input.poll_next_unpin(cx));
+ // Start timing the operation. The timer records time upon being dropped.
+ let _timer = cloned_time.timer();
+
+ match input_batch {
+ None => {
+ // Input stream is exhausted, finalize any remaining batches
+ self.completed = true;
+ self.coalescer.finish()?;
}
- CoalesceBatchesStreamState::Exhausted => {
- // Handle the end of the input stream.
- return if self.coalescer.is_empty() {
- // If buffer is empty, return None indicating the stream is fully consumed.
- Poll::Ready(None)
- } else {
- let _timer = cloned_time.timer();
- // If the buffer still contains batches, prepare to return them.
- let batch = self.coalescer.finish_batch()?;
- Poll::Ready(Some(Ok(batch)))
- };
+ Some(Ok(batch)) => {
+ if self.coalescer.push_batch(batch)? {
Review Comment:
I found this API to be somewhat confusing (the fact that a `true` return value means limit was reached)
Maybe returning an enum would be clearer here
I don't think this is a correctness issue, just a readability thing I noticed
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3170558819
๐ค `./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-1016-gcp #16~24.04.1-Ubuntu SMP Wed May 28 02:40:52 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing revive_to_use_upstream_arrow_coalesce (bac019798ecd83a9d1239d8ff06a3be4825723fb) to 9264bb88ab73ce64873dd1797df39453e41c2aad [diff](https://github.com/apache/datafusion/compare/9264bb88ab73ce64873dd1797df39453e41c2aad..bac019798ecd83a9d1239d8ff06a3be4825723fb) using: 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: 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: Revive to use upstream arrow coalesce [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3182761359
> For the `tpch_mem` slowdown, another possible reason could be unnecessary copies for batches that are exactly `batch_size`.
>
> For certain operators, there might already be an internal mechanism to ensure their output is exactly batch_size. From a quick look at the implementation, the old version could pass such batches through directly, whereas this PR forces them to be copied.
>
> Another potential improvement: could we make this pass-through threshold more lenient? For example, if the coalescer receives a batch with size >= `batch_size / 2`, it could pass it through without coalescing. In such cases, the output size is already large enough to benefit from vectorization, so the extra concatenation might not add much value.
Thank you @2010YOUY01 for review, good suggestion!
It looks like similar to this comments:
https://github.com/apache/datafusion/pull/16249#issuecomment-2974721937
I will try to address it, and we can get the new benchmark 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: Revive to use upstream arrow coalesce [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3177741480
Updated @alamb , i created the PR for non-exact size now:
https://github.com/apache/datafusion/pull/17136
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3183325134
Updated: addressed the comments from @2010YOUY01 , may be we can trigger a new benchmark to see the result @alamb, thanks a lot!
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3174912170
> > > ๐ค the new kernel seems to slow down. I wonder if the overhead of precisely sized output batches is causing the issue
> >
> >
> > Good point @alamb , i agree this is the only difference. I can add a test PR to make upstream do not generate precisely sized output batches, but when we ensure capacity for the increment buffer size, it seems we need to make the size change since we do not keep the same target size for this change.
> > The latest benchmark seems a little better.
> > >
>
> Thanks @zhuqi-lucas -- what I was thinking about was something like the following
>
> ```rust
> let target_batch_size = 4;
> let mut coalescer = BatchCoalescer::new(batch1.schema(), 4)
> .with_exact_size(false)
> ```
>
> Before we spend a lot of time polishing / testing a PR for that it would probably be good to hack up a POC and verify it actually improves performance
>
> Thank you for your willingness to help along with this project. It is something I have thought was important (but not critical) for a long time and so having someone to help really makes a big difference
Thank you @alamb for good suggestion! It looks pretty cool to me, and a config for this is very clever idea.
```rust
let target_batch_size = 4;
let mut coalescer = BatchCoalescer::new(batch1.schema(), 4)
.with_exact_size(false)
```
I will try to address this for upstream first, so we can easily testing it for datafusion.
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3174515574
๐ค: Benchmark completed
<details><summary>Details</summary>
<p>
```
Comparing HEAD and revive_to_use_upstream_arrow_coalesce
--------------------
Benchmark tpch_mem_sf1.json
--------------------
โโโโโโโโโโโโโโโโณโโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโ
โ Query โ HEAD โ revive_to_use_upstream_arrow_coalesce โ Change โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ QQuery 1 โ 95.61 ms โ 92.56 ms โ no change โ
โ QQuery 2 โ 20.85 ms โ 22.44 ms โ 1.08x slower โ
โ QQuery 3 โ 31.93 ms โ 36.28 ms โ 1.14x slower โ
โ QQuery 4 โ 18.15 ms โ 19.48 ms โ 1.07x slower โ
โ QQuery 5 โ 48.85 ms โ 53.88 ms โ 1.10x slower โ
โ QQuery 6 โ 12.03 ms โ 11.62 ms โ no change โ
โ QQuery 7 โ 88.27 ms โ 93.17 ms โ 1.06x slower โ
โ QQuery 8 โ 24.33 ms โ 24.10 ms โ no change โ
โ QQuery 9 โ 53.84 ms โ 55.02 ms โ no change โ
โ QQuery 10 โ 40.33 ms โ 41.29 ms โ no change โ
โ QQuery 11 โ 11.20 ms โ 11.65 ms โ no change โ
โ QQuery 12 โ 29.78 ms โ 29.46 ms โ no change โ
โ QQuery 13 โ 25.89 ms โ 26.05 ms โ no change โ
โ QQuery 14 โ 9.66 ms โ 10.02 ms โ no change โ
โ QQuery 15 โ 19.14 ms โ 18.82 ms โ no change โ
โ QQuery 16 โ 17.42 ms โ 17.74 ms โ no change โ
โ QQuery 17 โ 96.53 ms โ 95.86 ms โ no change โ
โ QQuery 18 โ 179.04 ms โ 179.63 ms โ no change โ
โ QQuery 19 โ 24.01 ms โ 26.11 ms โ 1.09x slower โ
โ QQuery 20 โ 31.57 ms โ 33.97 ms โ 1.08x slower โ
โ QQuery 21 โ 139.56 ms โ 151.41 ms โ 1.08x slower โ
โ QQuery 22 โ 13.78 ms โ 14.77 ms โ 1.07x slower โ
โโโโโโโโโโโโโโโโดโโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโ
โ Benchmark Summary โ โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ Total Time (HEAD) โ 1031.76ms โ
โ Total Time (revive_to_use_upstream_arrow_coalesce) โ 1065.32ms โ
โ Average Time (HEAD) โ 46.90ms โ
โ Average Time (revive_to_use_upstream_arrow_coalesce) โ 48.42ms โ
โ Queries Faster โ 0 โ
โ Queries Slower โ 9 โ
โ Queries with No Change โ 13 โ
โ Queries with Failure โ 0 โ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโ
```
</p>
</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: 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: Revive to use upstream arrow coalesce [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3188823065
> Man this one is tricky -- like many performance optimization quests, it will likely take careful engineering and investigation. It is fascinating to me that what in theory should be a net neutral change is turning out to be slower in some cases
Thank you @alamb , i still not give up until now:
Submitted another try here:
https://github.com/apache/datafusion/pull/17193
Can i get a benchmark for this PR, thanks a lot!
--
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: Revive to use upstream arrow coalesce [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17105:
URL: https://github.com/apache/datafusion/pull/17105#issuecomment-3214345808
superceded by https://github.com/apache/datafusion/pull/17193
--
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