Window Functions Order Conservation -- Follow-up On Set Monotonicity #14813
Conversation
github-actions
bot
added
physical-expr
| @@ -222,208 +227,6 @@ async fn test_remove_unnecessary_sort5() -> Result<()> { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
There was a problem hiding this comment.
Coverage of these tests are preserved in test_window_partial_constant_and_set_monotonicity()
berkaysynnada
force-pushed
the
follow-up/monotonic
branch
from
54a53ed to
ace28b6
Compare
berkaysynnada
force-pushed
the
follow-up/monotonic
branch
from
ace28b6 to
9b55465
Compare
| " BoundedWindowAggExec: wdw=[count: Ok(Field { name: \"count\", data_type: Int64, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {} }), frame: WindowFrame { units: Range, start_bound: Preceding(NULL), end_bound: CurrentRow, is_causal: false }], mode=[Sorted]", | ||
| " SortExec: expr=[nullable_col@0 ASC, non_nullable_col@1 ASC], preserve_partitioning=[false]", | ||
| " DataSourceExec: partitions=1, partition_sizes=[0]"]; | ||
| " SortExec: expr=[nullable_col@0 ASC, non_nullable_col@1 ASC], preserve_partitioning=[false]", |
There was a problem hiding this comment.
To the reviewers, please double check here. While 2nd BoundedWindowAggExec requires [nullable_col, non_nullable_col] ordering, it is not guaranteed in the previous version. If I'm not missing something, now it is, with a newly added partial sort.
There was a problem hiding this comment.
To me, it seems like we had a bug before which is now fixed.
| @@ -2280,3 +2086,1265 @@ async fn test_not_replaced_with_partial_sort_for_unbounded_input() -> Result<()> | |||
| assert_optimized!(expected_input, expected_no_change, physical_plan, true); | |||
| Ok(()) | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| async fn test_window_partial_constant_and_set_monotonicity() -> Result<()> { | |||
There was a problem hiding this comment.
Writing this with rs-test seems hard to debug, and the relation between the arguments and initial-final plans is not very clear. So I prefer this
| @@ -498,6 +697,15 @@ pub fn get_window_mode( | |||
| None | |||
| } | |||
|
|
|||
| fn all_possible_sort_options(expr: Arc<dyn PhysicalExpr>) -> Vec<PhysicalSortExpr> { | |||
There was a problem hiding this comment.
This can be init once maybe
|
PTAL @ozankabak, @alamb |
github-actions
bot
added
sql
ozankabak
force-pushed
the
follow-up/monotonic
branch
from
e698fef to
8425c17
Compare
github-actions
bot
removed
documentation
There was a problem hiding this comment.
I reviewed both the code and the tests carefully, and sent a commit for some improvements. This PR greatly improves the test coverage and fills some gaps in terms of set monotonicity optimizations.
I will wait for some more time before merging in case we get more eyes on this (which would be great).
|
I will go ahead and merge this since it is a follow-up to a previously discussed (and reviewed) PR/feature. It would still be great to have some post-merge review on this when you have some time on your hands @alamb. |
THanks -- I'll try to to look at it but I am currently quite tied up with triyng to get DataFusion 46 ready for release (and dealing with some unexpected fallout from #14224). May not be until next week |
Which issue does this PR close?
Rationale for this change
#14271 had introduced set-monotonicity term for window functions. After that PR we have some false negatives for order conservation, and we don't have a fully complete test coverage for window functions in terms of set-monotonicity, partitioning type, frame type, and order of the function inputs.
What changes are included in this PR?
This PR enriches the ordering properties of WindowAggExec and BoundedWindowAggExec, and I've written a test function which covers all cases for a window function, both positive and negative cases.
Are these changes tested?
Yes
Are there any user-facing changes?
Order conservation in more cases