Dandandan commented on code in PR #15380:
URL: https://github.com/apache/datafusion/pull/15380#discussion_r2096369131
##########
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:
Shouldn't we remove this `self.expr.len() <= 2` here? This wasn't here before
--
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...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
