Calculate ordering equivalence for expressions (rather than just columns) #8281

mustafasrepo · 2023-11-20T14:41:02Z

Which issue does this PR close?

Improves the situation on #8064

Rationale for this change

Currently we cannot determine whether ordering requirement of the complex expressions are satisfied.
Consider the case where requirement is [a+b ASC], and we know that existing orderings are [a ASC], [b ASC]. It should be possible to understand that given [a ASC] and [b ASC], requirement: [a+b ASC] is satisfied (Please note that we already have mechanism to make deduction. However, it is not considered in requirement satisfaction analysis. Currently we search requirement among existing orderings, this limits our ability during analysis).

What changes are included in this PR?

This PR re-implements the find_longest_permutation and ordering_satisfy_requirement so that ordering of the complex(composed) expressions are calculated correctly, and considered during analysis. Most of the changes comes from the tests, and test related changes.

Are these changes tested?

Yes.

Are there any user-facing changes?

* Discover ordering of complex expressions in group by and window partition by * Remove unnecessary tests * Update comments * Minor changes * Better projection support complex expression support * Fix failing test * Simplifications * Simplifications * Add is end flag * Simplifications * Simplifications * Simplifications * Minor changes * Minor changes * Minor changes * All tests pass * Change implementation of find_longest_permutation * Minor changes * Minor changes * Remove projection section * Remove projection implementation * Fix linter errors * Remove projection sections * Minor changes * Add docstring comments * Add comments * Minor changes * Minor changes * Add comments * simplifications

ozankabak · 2023-11-21T15:41:54Z

I will do another deep dive on this tomorrow

alamb · 2023-11-21T20:44:00Z

Thank you @mustafasrepo -- I plan to review this carefully tomorrow

ozankabak

Took a third pass today and LGTM

alamb

Thank you for this PR @mustafasrepo -- can you explain how non monotonic functions are handled (like extract(month from ts)) where the expressions don't preserve the ordering of their inputs? I didn't see any tests for such functions or code that handles them

Sorry I may have just missed it / don't understand the code

alamb · 2023-11-22T14:25:56Z

datafusion/core/src/physical_optimizer/replace_with_order_preserving_variants.rs

@@ -357,15 +357,19 @@ mod tests {
        let physical_plan =
            sort_preserving_merge_exec(vec![sort_expr("a", &schema)], sort);

-        let expected_input = ["SortPreservingMergeExec: [a@0 ASC NULLS LAST]",
+        let expected_input = [


Did these plans actually change? Or is this just whitespace changes?

If it is just whitespace changes I would really appreciate breaking such changes out into their own PRs as they are much faster / easier to review and merge.

Just whitespace changes. Agree with your suggestion in general, we will try to improve our self discipline on that

alamb · 2023-11-22T14:26:32Z

datafusion/core/src/physical_optimizer/enforce_distribution.rs

@@ -3787,7 +3787,7 @@ pub(crate) mod tests {
    fn repartition_transitively_past_sort_with_projection_and_filter() -> Result<()> {
        let schema = schema();
        let sort_key = vec![PhysicalSortExpr {
-            expr: col("c", &schema).unwrap(),
+            expr: col("a", &schema).unwrap(),


why was this test changed?

In the plan, there is a filter enforcing c = 0 -- so having a sort on c is a no-op logically. After recent improvements we are able to recognize things like that and remove sorts completely, so the test became moot in its old form.

alamb · 2023-11-22T14:30:46Z

datafusion/sqllogictest/test_files/groupby.slt

+query P
+SELECT date_bin('15 minutes', ts) as time_chunks
+  FROM csv_with_timestamps
+  GROUP BY (date_bin('15 minutes', ts))


I am confused about the seeming extra ( and ) here and in the above query:

Is there any reason it is not:

Suggested change

GROUP BY (date_bin('15 minutes', ts))

GROUP BY date_bin('15 minutes', ts)

No reason, it was unnecessary -- now removed

alamb · 2023-11-22T14:33:10Z

datafusion/sqllogictest/test_files/groupby.slt

+----
+2018-12-13T12:00:00
+2018-11-13T17:00:00
+


Could you also please add a negative test that can not work in streaming mode with an expression that doesn't preserve the order?

perhaps something like

SELECT extract(month from ts) as months, FROM csv_with_timestamps GROUP BY extract(month from ts) ORDER BY time_chunks DESC LIMIT 5

Good idea. I added a test that shows the query works when the function is monotonic but not otherwise for unbounded sources. I also added a test that shows the query works regardless of the function's monotonicity for bounded sources.

ozankabak · 2023-11-22T20:33:10Z

Thanks for the review @alamb. I answered your questions and added a couple tests to address your suggestions (verifying behavior difference for monotonic/nonmonotonic functions). Regarding your question, complex expression orderings are calculated via the get_expr_ordering/update_ordering functions which handle monotonic/nonmonotonic expressions.

alamb

Thanks @ozankabak -- I didn't review the logic changes carefully but I don't think I need to either given you have. The tests added to datafusion/sqllogictest/test_files/groupby.slt look great to me and convince me this works correctly

ozankabak · 2023-11-22T20:44:05Z

For some reason sqllogictests are failing in CI (they pass locally for me). Looking at the logs, it seems like the expected/actual are the same, so I can't figure out why this is the case. I'll let @mustafasrepo figure it out and fix tomorrow and we can merge then.

In the meantime if there is more suggestions for improvements from the community we will incorporate them as well

mustafasrepo and others added 5 commits November 17, 2023 08:59

Minor changes

b35d343

Review Part 1

bb77516

Add new tests

b5f0ef8

Review Part 2

8ee9f1e

github-actions bot added physical-expr Physical Expressions core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Nov 20, 2023

alamb mentioned this pull request Nov 20, 2023

DataFusion weekly project plan (Andrew Lamb) - Nov 20, 2023 #8274

Closed

7 tasks

alamb changed the title ~~Complex exprs requirement support~~ Calculate ordering equivalence for expressions (rather than just columns) Nov 22, 2023

ozankabak approved these changes Nov 22, 2023

View reviewed changes

alamb reviewed Nov 22, 2023

View reviewed changes

Address review feedback

3319c8d

alamb approved these changes Nov 22, 2023

View reviewed changes

Remove error message check in the test

13dcd6c

ozankabak merged commit 9619f02 into apache:main Nov 23, 2023
22 checks passed

mustafasrepo mentioned this pull request Nov 29, 2023

Add support for utilization complex aggregate exprs inside group by #8107

Closed

mustafasrepo mentioned this pull request Dec 9, 2023

Better Equivalence (ordering and exact equivalence) Propagation through ProjectionExec #8484

Merged

matthewgapp mentioned this pull request Jan 11, 2024

matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calculate ordering equivalence for expressions (rather than just columns) #8281

Calculate ordering equivalence for expressions (rather than just columns) #8281

mustafasrepo commented Nov 20, 2023

ozankabak commented Nov 21, 2023

alamb commented Nov 21, 2023

ozankabak left a comment

alamb left a comment

alamb Nov 22, 2023

ozankabak Nov 22, 2023

alamb Nov 22, 2023

ozankabak Nov 22, 2023

alamb Nov 22, 2023

ozankabak Nov 22, 2023

alamb Nov 22, 2023

ozankabak Nov 22, 2023

ozankabak commented Nov 22, 2023

alamb left a comment •

edited

Loading

ozankabak commented Nov 22, 2023 •

edited

Loading

	GROUP BY (date_bin('15 minutes', ts))
	GROUP BY date_bin('15 minutes', ts)

Calculate ordering equivalence for expressions (rather than just columns) #8281

Calculate ordering equivalence for expressions (rather than just columns) #8281

Conversation

mustafasrepo commented Nov 20, 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?

ozankabak commented Nov 21, 2023

alamb commented Nov 21, 2023

ozankabak left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozankabak commented Nov 22, 2023

alamb left a comment • edited Loading

Choose a reason for hiding this comment

ozankabak commented Nov 22, 2023 • edited Loading

alamb left a comment •

edited

Loading

ozankabak commented Nov 22, 2023 •

edited

Loading