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

mustafasrepo · 2023-03-29T08:08:39Z

Which issue does this PR close?

N.A

Note

The changes in this PR is received from the #5290 by @mingmwang.

Rationale for this change

For some executors it is enough that its input is ordered, however direction of ordering isn't important (such as PARTITION BY columns in the WindowAggExec). Current required_input_ordering API Vec<Option<&[PhysicalSortExpr]>> doesn't supports this subtlety. Where PhysicalSortExpr is a struct encapsulates expr: Arc<dyn PhysicalExpr> and options: SortOptions.

What changes are included in this PR?

In this PR we change required_input_ordering
from fn required_input_ordering(&self) -> Vec<Option<&[PhysicalSortExpr]>>
to fn required_input_ordering(&self) -> Vec<Option<Vec<PhysicalSortRequirement>>>, where PhysicalSortRequirement is a struct encapsulates expr: Arc<dyn PhysicalExpr> and options: Option<SortOptions>. If options field is None. It means that executor expects its input to be ordered but direction doesn't not matter.
Also some util functions to convert in between these structs are added.

Are these changes tested?

Existing tests should work.

Are there any user-facing changes?

api change

…rection.

mingmwang · 2023-03-29T08:22:50Z

LGTM

mingmwang · 2023-03-29T10:22:10Z

datafusion/core/src/physical_plan/windows/mod.rs

+pub(crate) fn calc_requirements(
+    partition_by_exprs: &[Arc<dyn PhysicalExpr>],
+    orderby_sort_exprs: &[PhysicalSortExpr],
+) -> Option<Vec<PhysicalSortRequirement>> {
+    let mut sort_reqs = vec![];
+    for partition_by in partition_by_exprs {
+        sort_reqs.push(PhysicalSortRequirement {
+            expr: partition_by.clone(),
+            options: None,
+        });
+    }
+    for PhysicalSortExpr { expr, options } in orderby_sort_exprs {
+        let contains = sort_reqs.iter().any(|e| expr.eq(&e.expr));
+        if !contains {
+            sort_reqs.push(PhysicalSortRequirement {
+                expr: expr.clone(),
+                options: Some(*options),
+            });
+        }
+    }
+    // Convert empty result to None. Otherwise wrap result inside Some()
+    (!sort_reqs.is_empty()).then_some(sort_reqs)
+}
+


If there is overlap between partition by keys and sort keys, I think we should respect the sort key's requirements.

In the case of PARTITION BY a, ORDER BY a. We do not add requirement for column a. The reason is that all partition would consist of the same value of a. Hence ORDER BY doesn't really define a direction(all a values would be same.). Above operation can produce correct result when its input is ordered according a ASC or a DESC.

But if the original SQL is PARTITION BY a, ORDER BY a DESC, should we respect the direction? I'm not sure for this.
It think for cases like ROWNUM() over, the direction matters.

Since ORDER BY is local to each partition, I think we are fine here. There is some related discussion here: https://stackoverflow.com/questions/50364818/using-the-same-column-in-partition-by-and-order-by-with-dense-rank

I am still taking a note to remind us that we may want to revisit this if we find out information indicating otherwise.

Computing rownum() over PARTITION BY a, ORDER BY a DESC I think will result in arbitrary assignments of row numbers (as @ozankabak and @mustafasrepo say above, the value of a within each partition is the same)

mingmwang · 2023-03-29T10:30:47Z

datafusion/physical-expr/src/sort_expr.rs

+            })
+    }
+}
+


alamb

Thank you @mingmwang and @mustafasrepo

FWIW I will try and update IOx to use this new API shortly after the PR is merged. I found it challenging last time (see related discussion https://github.com/apache/arrow-datafusion/pull/5661/files#r1148410281) as the signature

Vec<Option<Vec<PhysicalSortRequirement>>>

Imposes a significant cognitive load. However given this PR moves things forward and we don't have a competing alternate proposal I think we should merge the API as is.

alamb · 2023-03-29T19:31:38Z

datafusion/core/src/physical_plan/windows/mod.rs

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


Is it correct that being able to express options:None for partitioning operations is the key rationale (thing we can't do before) for this PR?

Right. I vaguely remember us discussing internally that there are also some other use cases where some sort of ordering is required but the options don't matter, but I can't recall now

Another example that the sorting direction is not that important is the SortMergeJoin, but the additional requirements
is for all its input, the ordering should be aligned, should be ASC or DESC altogether. I think the current PhysicalSortRequirement API can not represent this alignment constraints explicitly.

The other example is SortBasedAggregator, currently we do not support SortBasedAggregator in DataFusion

alamb · 2023-03-29T19:33:32Z

datafusion/core/src/physical_plan/windows/mod.rs

+pub(crate) fn calc_requirements(
+    partition_by_exprs: &[Arc<dyn PhysicalExpr>],
+    orderby_sort_exprs: &[PhysicalSortExpr],
+) -> Option<Vec<PhysicalSortRequirement>> {
+    let mut sort_reqs = vec![];
+    for partition_by in partition_by_exprs {
+        sort_reqs.push(PhysicalSortRequirement {
+            expr: partition_by.clone(),
+            options: None,
+        });
+    }
+    for PhysicalSortExpr { expr, options } in orderby_sort_exprs {
+        let contains = sort_reqs.iter().any(|e| expr.eq(&e.expr));
+        if !contains {
+            sort_reqs.push(PhysicalSortRequirement {
+                expr: expr.clone(),
+                options: Some(*options),
+            });
+        }
+    }
+    // Convert empty result to None. Otherwise wrap result inside Some()
+    (!sort_reqs.is_empty()).then_some(sort_reqs)
+}
+


Computing rownum() over PARTITION BY a, ORDER BY a DESC I think will result in arbitrary assignments of row numbers (as @ozankabak and @mustafasrepo say above, the value of a within each partition is the same)

alamb · 2023-03-29T19:34:45Z

datafusion/physical-expr/src/utils.rs

+
+/// Checks whether the given [`PhysicalSortRequirement`]s are satisfied by the
+/// provided [`PhysicalSortExpr`]s.
+pub fn ordering_satisfy_requirement_concrete<F: FnOnce() -> EquivalenceProperties>(


Is this meant to be pub? It seems like callers should always use ordering_satisfy_requirement, right?

Although it is not absolutely necessary for both ordering_satisfy_requirement and ordering_satisfy_requirement_concrete to be public. Their API is different, we may want to use both of them. Actually, sort_enforcement uses ordering_satisfy_requirement_concrete.

ozankabak · 2023-03-29T19:44:43Z

Thanks @alamb -- I will create a separate follow-on that leverages type aliases to simplify things after this and its sister PR merges.

alamb · 2023-03-29T19:59:49Z

I'll plan to merge this tomorrow unless anyone else would like time to comment

alamb · 2023-04-04T16:25:23Z

Thanks @alamb -- I will create a separate follow-on that leverages type aliases to simplify things after this and its sister PR merges.

I have made a PR #5863 that doesn't change the type signatures but I think will make using this new structure easier.

Change required input ordering to format to not absolutely require di…

1f75c4c

…rection.

github-actions bot added core Core DataFusion crate physical-expr Physical Expressions labels Mar 29, 2023

mustafasrepo changed the title ~~Change required input ordering format~~ Change required input ordering API Mar 29, 2023

mustafasrepo mentioned this pull request Mar 29, 2023

Top down EnforceSorting, Extended testbench for EnforceSorting rule to prepare for refactors, additional functionality such as pushdowns over unions #5661

Merged

mingmwang reviewed Mar 29, 2023

View reviewed changes

datafusion/physical-expr/src/sort_expr.rs

})

}

}

Copy link

Contributor

mingmwang Mar 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

alamb added the api change Changes the API exposed to users of the crate label Mar 29, 2023

alamb changed the title ~~Change required input ordering API~~ Change required input ordering API to allow any NULLS FIRST / LAST and ASC / DESC Mar 29, 2023

alamb approved these changes Mar 29, 2023

View reviewed changes

remove unnecessary code

45f03e4

alamb changed the title ~~Change required input ordering API to allow any NULLS FIRST / LAST and ASC / DESC~~ Change required input ordering physical plan API to allow any NULLS FIRST / LAST and ASC / DESC Mar 30, 2023

alamb merged commit c9bf3f3 into apache:main Mar 30, 2023

alamb mentioned this pull request Apr 4, 2023

Encapsulate PhysicalSortRequrement and add more doc comments #5863

Merged

mustafasrepo deleted the feature/exec_plan_req_ordering branch April 11, 2023 06:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mustafasrepo commented Mar 29, 2023 •

edited

Loading

mingmwang commented Mar 29, 2023

mingmwang Mar 29, 2023

mustafasrepo Mar 29, 2023

mingmwang Mar 29, 2023

ozankabak Mar 29, 2023

alamb Mar 29, 2023

mingmwang Mar 29, 2023

alamb left a comment

alamb Mar 29, 2023

ozankabak Mar 30, 2023

mingmwang Mar 30, 2023

mingmwang Mar 30, 2023

alamb Mar 29, 2023

alamb Mar 29, 2023

mustafasrepo Mar 30, 2023 •

edited

Loading

ozankabak commented Mar 29, 2023

alamb commented Mar 29, 2023

alamb commented Apr 4, 2023

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

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

Conversation

mustafasrepo commented Mar 29, 2023 • edited Loading

Which issue does this PR close?

Note

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

mingmwang commented Mar 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustafasrepo Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

ozankabak commented Mar 29, 2023

alamb commented Mar 29, 2023

alamb commented Apr 4, 2023

mustafasrepo commented Mar 29, 2023 •

edited

Loading

mustafasrepo Mar 30, 2023 •

edited

Loading