gabotechs commented on code in PR #18673:
URL: https://github.com/apache/datafusion/pull/18673#discussion_r2524694169
##########
datafusion/physical-plan/src/repartition/mod.rs:
##########
@@ -1178,7 +1180,8 @@ impl RepartitionExec {
/// Return the sort expressions that are used to merge
fn sort_exprs(&self) -> Option<&LexOrdering> {
- if self.preserve_order {
+ if self.preserve_order ||
self.input.output_partitioning().partition_count() <= 1
+ {
Review Comment:
Note that this is not only changing display details, this is actually
changing the behavior of `RepartitionExec` itself.
During execution, `RepartitionExec` will look at the return of this function
in order to know if it should preserve partitioning by evaluating the
`sort_exprs` expressions:
https://github.com/apache/datafusion/blob/main/datafusion/physical-plan/src/repartition/mod.rs#L839-L839
Which means that adding this
`self.input.output_partitioning().partition_count() <= 1` not only changes
display, but will also probably perform unnecessary expression evaluations.
Not sure if this was intentional, but my guess is that no `sort_exprs`
should be evaluated if `self.preserver_order == false`
##########
datafusion/physical-plan/src/repartition/mod.rs:
##########
@@ -737,18 +737,21 @@ impl RepartitionExec {
impl DisplayAs for RepartitionExec {
fn fmt_as(&self, t: DisplayFormatType, f: &mut Formatter) ->
std::fmt::Result {
+ let input_partition_count =
self.input.output_partitioning().partition_count();
match t {
DisplayFormatType::Default | DisplayFormatType::Verbose => {
write!(
f,
"{}: partitioning={}, input_partitions={}",
self.name(),
self.partitioning(),
- self.input.output_partitioning().partition_count()
+ input_partition_count,
)?;
if self.preserve_order {
write!(f, ", preserve_order=true")?;
+ } else if input_partition_count <= 1 {
+ write!(f, ", maintains_sort_order=true")?;
Review Comment:
I get the impression that this `maintains_sort_order` here might be
confusing, as it's not a real property of `RepartitionExec`, and instead is
something artificially derived from other properties that are already
displayed, and therefore, redundant.
--
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]
