zhuqi-lucas commented on code in PR #8146:
URL: https://github.com/apache/arrow-rs/pull/8146#discussion_r2280462502


##########
arrow-select/src/coalesce.rs:
##########
@@ -236,6 +262,13 @@ impl BatchCoalescer {
     /// assert_eq!(completed_batch, expected_batch);
     /// ```
     pub fn push_batch(&mut self, batch: RecordBatch) -> Result<(), ArrowError> 
{
+        if let Some(limit) = self.biggest_coalesce_batch_size {

Review Comment:
   > Also, thinking about it, we can as well prevent flushing in progress array 
if we do not require the output to be sorted (so smaller in progress batches 
will still be combined into a output batch of the target size).
   
   Great point @Dandandan  , i agree this is a todo, i was also thinking, even 
we skip coalesce large batch, we will still output smaller batches before the 
large batch. If we don't need sort, we can combine all smaller batches, but the 
logic may need more investigation(how to define and get sort flag in low level, 
etc).
   
   > I mean if there are already smaller batches in the in progress array and 
the condition holds, the array will be created and in progress batches will be 
copied, creating two batches.
   It would be worth testing I think only having the short path if the 
condition holds and there are no (smaller) in progress batches, so there is 
only one output batch being generated.
   
   Interesting, so if i make sense right, there are two cases for it, for 
example we target batch size is 1000, input batches:
   
   1. 20, 20, 700, 20, 30, 700, 30, 20, 10, 15
   2. 20, 20, 30, 700, 600, 700, 900, 700, 600
   
   
   So we can only apply short path for continue large batch, the case 2, 
because when we meet continue large batches, we will not have progress batches, 
so we can output directly.
   
   But for case 1, we will not apply the short path, we will keep to coalesce 
it, because before each large batch, we already have small batches in progress, 
so we don't  apply short path.
   
   Am i right? I am not sure the performance result, i need to testing with 
datafusion, 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-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to