ruchirK commented on PR #18673:
URL: https://github.com/apache/datafusion/pull/18673#issuecomment-3530488263
> I have to say I'm not fully convinced about this change. The extra added
information is redundant with information already present in the plan display,
as it's not really adding anything new, and it can make the plans more verbose
without adding any real information.
>
> I'd love to weight opinions of other contributors though.
Thank you for the thoughtful and prompt review @gabotechs ! My understanding
of the original task was that in some cases it might be helpful to display some
redundant information to make plan diagrams easier to parse at a glance.
The way I see it, there's two related questions here.
1. Should we display any additional information when the `RepartitionExec`
operator implicitly preserves sortedness? There's at least 3 answers here.
a. No, we already display `input_partitions=1` which is sufficient.
b. Yes, but only in the implicit case. Display `maintains_sort_order=true`
when there's only 1 partition. If `preserve_order` is true just display that,
and don't show `maintains_sort_order`. That's the intended behavior of this PR.
c. Yes, but go further. Fully remove `preserve_sort_order=true` and only
display `maintains_sort_order`. This would, as mentioned in the issue, conflate
the implicit and explicit decisions.
2. When sortedness is implicitly maintained in the single partition case,
should we show something in the plan diagram indicating what the implicitly
maintained sort order is? Note that this is a little bit independent of
question 1 and this was my intent with the `sort_exprs` change.
My personal take is to generally agree with you that these changes are not
super big usability wins. I'd love to here @gene-bordegaray 's take as well.
For now, I'll update the PR to remove the `sort_exprs` change and then we can
go from there.
--
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]
