Encapsulate `PhysicalSortRequrement` and add more doc comments #5863

alamb · 2023-04-04T13:26:54Z

Which issue does this PR close?

Rationale for this change

As I mentioned in #5772 (review) , I found the cognitive load of dealing with Vec<Option<Vec<PhysicalSortRequirements>>>> challenging and the fact that PhysicalSortRequirements had another Option inside it was also challenging when reading the code .

Also, it appears that the make_sort_requirements_from_exprs function was not public outside of the datafusion-physical-expr crate (so I could not used it while updating IOx -- see https://github.com/influxdata/influxdb_iox/pull/7440). It isn't hard to rewrite but I wanted to help others potentially upgrading by adding more comments and hints of how to create this type of structure.

I also think having additional documentation (and method names that are self documenting) adding methods makes the intent clearer when reading.

What changes are included in this PR?

Make fields on PhysicalSortRequirement private
Add doc strings and examples of what the different options are, and the relevant constructors
Add From impls

Are these changes tested?

Covered by existing tests

Are there any user-facing changes?

alamb · 2023-04-04T13:31:01Z

datafusion/physical-expr/src/utils.rs

-                    options,
-                }
-            } else {
-                PhysicalSortExpr {


moved into PhysicalSortExpr::into_sort_expr() where I think it is easier to find and understand (turns out converting back / forth from PhysicalSortExpr and PhysicalSortRequirement is common

alamb · 2023-04-04T13:34:21Z

datafusion/core/src/physical_plan/sorts/sort_preserving_merge.rs


    fn required_input_ordering(&self) -> Vec<Option<Vec<PhysicalSortRequirement>>> {
-        vec![Some(make_sort_requirements_from_exprs(&self.expr))]
+        vec![Some(PhysicalSortRequirement::exact_from_exprs(&self.expr))]


I am imagining that when a user upgrades DataFusion they will and see that the type signature changed from PhysicalSortExpr to PhysicalSortRequirement. Then when they look at PhysicalSortRequirement they can find a function to convert the existing PhysicalSortExpr to PhysicalSortRequirement -- PhysicalSortRequirement::exact_from_exprs

make_sort_requirements_from_exprs was both not documented as well as lost in the list of functions

ozankabak · 2023-04-04T13:43:09Z

This LGTM. However, let's discuss timing. We have a type alias PR that is waiting for #5661. IMO we should do this after the alias PR as de-conflicting that may be much harder than de-conflicting this.

alamb · 2023-04-04T13:49:40Z

IMO we should do this after the alias PR as de-conflicting that may be much harder than de-conflicting this.

I agree -- this one is a nice to have and has no particular urgency. Lets get your PRs merged first

ozankabak · 2023-04-04T16:39:52Z

The alias PR is #5867. I think apart from a simple name change of make_sort_requirements_from_exprs, it should be relatively easy to merge this with that. If we get these two merged up, the state of affairs on this should improve somewhat

ozankabak

Left a few minor comments but LGTM in general

ozankabak · 2023-04-04T17:32:52Z

datafusion/physical-expr/src/sort_expr.rs

    /// Option to specify how the given column should be sorted.
-    /// If unspecified, there is no constraint on sort options.
-    pub options: Option<SortOptions>,
+    /// If unspecified, there are constraints on sort options.


This comment seems incorrect, did you mean to start with "If specified, ..."?

Nice catch. I will fix it (restore to the original)

Update: fixed in a5cc4c2

ozankabak · 2023-04-04T17:35:56Z

datafusion/physical-expr/src/sort_expr.rs

+    /// Creates a new `exact` requirement, which must match the
+    /// required options and expression. See
+    /// [`PhysicalSortRequirement`] for examples.
+    pub fn new_exact(expr: Arc<dyn PhysicalExpr>, options: SortOptions) -> Self {
+        Self {
+            expr,
+            options: Some(options),
+        }
+    }
+
+    /// Creates a new `expr_only` requirement, which must match the
+    /// required expression. See [`PhysicalSortRequirement`] for
+    /// examples.
+    pub fn new_expr_only(expr: Arc<dyn PhysicalExpr>) -> Self {
+        Self {
+            expr,
+            options: None,
+        }
+    }


You could have a single new and the caller can pass the second argument as None if they don't want to constrain sort options. I get the sense that you have two news because you think it is more readable?

Yes - that is precisely the reason.

For example, I thought it was clearer that the goal was to add a relaxed sort requirement in datafusion/core/src/physical_plan/windows/mod.rs with:

for partition_by in partition_by_exprs { sort_reqs.push(PhysicalSortRequirement::new_expr_only(partition_by.clone())) }

Rather than

for partition_by in partition_by_exprs { sort_reqs.push(PhysicalSortRequirement { expr: partition_by.clone(), options: None, }); }

Of course they both do the same thing eventually and I agree it is a preference thing about what makes for more readable code

The alternative I had in mind was

for partition_by in partition_by_exprs { sort_reqs.push(PhysicalSortRequirement::new(partition_by.clone(), None)); // sort_reqs.push(PhysicalSortRequirement::new(partition_by.clone(), Some(sort_options))); }

I can make that change if you prefer it

Changed in 8a073c4

ozankabak · 2023-04-04T17:37:36Z

datafusion/physical-expr/src/sort_expr.rs

+    pub fn into_sort_expr(self) -> PhysicalSortExpr {
+        let Self { expr, options } = self;
+
+        let options = options.unwrap_or(SortOptions {


I'd suggest unwrap_or_else to explicitly avoid constructing a potentially unused object. The compiler will likely take care of this, but I find making intent visible in code structure is good practice.

Indeed -- I agree with you. I originally had unwrap_or_else for precisely the reasons you mentioned, but clippy told me to use unwrap_or instead 😭

error: unnecessary closure used to substitute value for `Option::None` --> datafusion/physical-expr/src/sort_expr.rs:185:23 | 185 | let options = options.unwrap_or_else(|| SortOptions { | _______________________^ 186 | | descending: false, 187 | | nulls_first: false, 188 | | }); | |__________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_lazy_evaluations = note: `-D clippy::unnecessary-lazy-evaluations` implied by `-D warnings` help: use `unwrap_or(..)` instead | 185 ~ let options = options.unwrap_or(SortOptions { 186 + descending: false, 187 + nulls_first: false, 188 ~ }); |

…_expr

alamb

Thank you for the review @ozankabak

Not sure if either @mustafasrepo or @mingmwang are interested in this PR either, given they have been working in this area

…_expr

mustafasrepo · 2023-04-06T07:53:43Z

datafusion/physical-expr/src/sort_expr.rs

+    pub fn to_sort_exprs(
+        requirements: impl IntoIterator<Item = PhysicalSortRequirement>,
+    ) -> Vec<PhysicalSortExpr> {


I think we can use

pub fn to_sort_exprs<'a>( requirements: impl IntoIterator<Item = &'a PhysicalSortRequirement>, ) -> Vec<PhysicalSortExpr>

as signature for to_sort_exprs as in from_sort_exprs. This would help remove a couple clones in the code.

My rationale for taking PhysicalSortRequirement rather than &PhysicalSortRequirement was actually to avoid a clone in the cases where the caller already had an owned PhysicalSortRequirement. Basically the clone() is moved from inside this function to the callsite

Specifically in datafusion/core/src/physical_optimizer/sort_enforcement.rs

// Make sure we preserve the ordering requirements: update_child_to_remove_unnecessary_sort(child, sort_onwards, &plan)?; let sort_expr = PhysicalSortRequirement::to_sort_exprs(required_ordering); add_sort_above(child, sort_expr)?; if is_sort(child) {

This signature can avoid a clone.

If the signature looks like

pub fn to_sort_exprs<'a>( requirements: impl IntoIterator<Item = &'a PhysicalSortRequirement>, ) -> Vec<PhysicalSortExpr>

I think it would require always clone()ing the expr and options internally to create an owned PhysicalSortExpr in the output

Of course, we are talking about cloning an Arc and some SortOptions which presumably isn't all that expensive , so maybe it doesn't matter 🤔

I have a feeling there may be a way to do this with a Borrow trick to implement a single function that can deal with both owned and referenced arguments without duplicating code 🤔

@alamb, it seems like using

pub fn to_sort_exprs<T: Borrow<PhysicalSortRequirement>>( requirements: impl IntoIterator<Item = T>, ) -> Vec<PhysicalSortExpr> { requirements .into_iter() .map(|e| PhysicalSortExpr::from(e.borrow())) .collect() } pub fn from_sort_exprs<T: Borrow<PhysicalSortExpr>>( ordering: impl IntoIterator<Item = T>, ) -> Vec<PhysicalSortRequirement> { ordering .into_iter() .map(|e| PhysicalSortRequirement::from(e.borrow())) .collect() }

lets us get rid of a bunch of clones and results in simpler call site code. AFAIK, this is a zero-cost mechanism. If you concur, we can use this pattern.

I created #11 on your repo so you can easily check it out and incorporate (assuming I'm not overlooking something and this is indeed zero cost obviously).

I guess my point was that I think this call:

PhysicalSortRequirement::from(e.borrow()

Invokes this From impl:

impl From<&PhysicalSortRequirement> for PhysicalSortExpr { fn from(value: &PhysicalSortRequirement) -> Self { value.clone().into_sort_expr() } }

Which has a clone of the value

I have removed the From<&PhysicalSortRequirement> traits in 12cecdb because it is obscuring the locations of the cloning.

I see. I thought you wanted to make to_sort_exprs "smart" in the sense that it can take work with an owned value or a reference, using the owned or reference From impl depending on the argument -- making a clone only when necessary.

Anyway I got curious and tried to verify whether the borrow mechanism was zero-cost for this purpose; it turns out it is not. It always calls the reference path. I also found how to do this -- here is an example for future reference.

For posterity, had we wanted to go this route, the smart to_sort_exprs would be:

pub fn to_sort_exprs<T>( requirements: impl IntoIterator<Item = T>, ) -> Vec<PhysicalSortExpr> where PhysicalSortExpr: From<T>, { requirements .into_iter() .map(|e| PhysicalSortExpr::from(e)) .collect() }

which would route to the right PhysicalSortExpr::from depending on the argument.

Very cool -- I did not know that. If you think that is a worthwhile change, I would be happy to make a follow on PR

I think it'd be a good change, I can already see some use cases for it in some upcoming code (e.g. streaming group bys).

I updated the mini-PR I created for experimentation so you can get the changes.

Thanks -- I tried to pull the commits into #5918 but they still seem to be trying to use this impl:

impl From<&PhysicalSortRequirement> for PhysicalSortExpr {

I may have messed up the merge somehow

Let's keep collaborating on #5918

mustafasrepo · 2023-04-06T08:00:34Z

datafusion/physical-expr/src/sort_expr.rs

+/// If the requirement is *exact*
+/// ([`PhysicalSortRequirement::new_exact`]), then the sort
+/// requirement will only be satisfied if it matches both the
+/// expression *and* the sort options.
+///
+/// If the requirement is *`expr_only`
+/// ([`PhysicalSortRequirement::new_expr_only`]) then only the expr
+/// must match, to satisfy the requirement.
+///


It seems that we now use PhysicalSortRequirement::new for initialization for both cases. These comments seem remainder for old API.

👍 that is a good spot -- fixed in 1db636a

mustafasrepo · 2023-04-06T08:02:15Z

I have made some suggestions. Besides these suggestions, LGTM!. Thanks @alamb.

…_expr

alamb · 2023-04-07T14:26:12Z

Thank you for the reviews and comments @mustafasrepo and @ozankabak

alamb marked this pull request as draft April 4, 2023 13:27

github-actions bot added core Core DataFusion crate physical-expr Changes to the physical-expr crates labels Apr 4, 2023

alamb commented Apr 4, 2023

View reviewed changes

alamb force-pushed the alamb/easier_physical_expr branch from 9f111b5 to 0602ae4 Compare April 4, 2023 14:49

Encapsulate PhysicalSortRequrement and add more doc comment

fc77409

alamb force-pushed the alamb/easier_physical_expr branch from 0602ae4 to fc77409 Compare April 4, 2023 15:51

alamb mentioned this pull request Apr 4, 2023

Change required input ordering physical plan API to allow any NULLS FIRST / LAST and ASC / DESC #5772

Merged

alamb marked this pull request as ready for review April 4, 2023 16:25

alamb mentioned this pull request Apr 4, 2023

Use type aliases ExprOrdering and OrderingRequirements to simplify ordering-related type signatures #5867

Closed

ozankabak approved these changes Apr 4, 2023

View reviewed changes

alamb added 2 commits April 5, 2023 09:01

fix a comment

a5cc4c2

Merge remote-tracking branch 'apache/main' into alamb/easier_physical…

39eca48

…_expr

alamb commented Apr 5, 2023

View reviewed changes

alamb added 2 commits April 5, 2023 14:56

simplify API

8a073c4

Merge remote-tracking branch 'apache/main' into alamb/easier_physical…

d18aa71

…_expr

mustafasrepo reviewed Apr 6, 2023

View reviewed changes

alamb added 2 commits April 6, 2023 17:01

Merge remote-tracking branch 'apache/main' into alamb/easier_physical…

0c0a2da

…_expr

Update comments to reflect new function

1db636a

alamb changed the title ~~Encapsulate PhysicalSortRequrement and add more doc comment~~ Encapsulate PhysicalSortRequrement and add more doc comments Apr 7, 2023

alamb added 2 commits April 7, 2023 07:15

Remove implicit cloning

12cecdb

Merge remote-tracking branch 'apache/main' into alamb/easier_physical…

50ed3b4

…_expr

alamb mentioned this pull request Apr 7, 2023

Prepare for 22.0.0 release #5916

Merged

alamb merged commit 10b9352 into apache:main Apr 7, 2023

Encapsulate PhysicalSortRequrement and add more doc comments #5863

Encapsulate PhysicalSortRequrement and add more doc comments #5863

Uh oh!

Conversation

alamb commented Apr 4, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

alamb Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ozankabak commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented Apr 4, 2023

Uh oh!

ozankabak commented Apr 4, 2023

Uh oh!

ozankabak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ozankabak Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ozankabak Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ozankabak Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ozankabak Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Encapsulate `PhysicalSortRequrement` and add more doc comments #5863

Encapsulate `PhysicalSortRequrement` and add more doc comments #5863

alamb Apr 4, 2023 •

edited

Loading

ozankabak commented Apr 4, 2023 •

edited

Loading

alamb Apr 5, 2023 •

edited

Loading

ozankabak Apr 5, 2023 •

edited

Loading

ozankabak Apr 4, 2023 •

edited

Loading

ozankabak Apr 6, 2023 •

edited

Loading

ozankabak Apr 7, 2023 •

edited

Loading