zhuqi-lucas commented on code in PR #15380:
URL: https://github.com/apache/datafusion/pull/15380#discussion_r2096766180
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -674,16 +676,35 @@ impl ExternalSorter {
return self.sort_batch_stream(batch, metrics, reservation);
}
- // If less than sort_in_place_threshold_bytes, concatenate and sort in
place
- if self.reservation.size() < self.sort_in_place_threshold_bytes {
- // Concatenate memory batches together and sort
- let batch = concat_batches(&self.schema, &self.in_mem_batches)?;
+ // If less than sort_in_place_threshold_bytes, we sort in memory.
+ // Note:
+ // In theory we should always be able to sort in place, but some
corner cases for merging testing
+ // failed, so we set a large threshold to avoid that.
+ // Also, we only support sort expressions with less than 3 columns for
now. Because from testing, when
+ // columns > 3, the performance of in-place sort is worse than
sort/merge.
+ // Need to further investigate the performance of in-place sort when
columns > 3.
+ if self.expr.len() <= 2
Review Comment:
Thank you @Dandandan for review, there was regression for testing >2 cases.
Addressed in latest PR, let's see the result for smallest changes. 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]
