Refactor into LexOrdering::collapse, LexRequirement::collapse avoid clone
#14038
Conversation
alamb
changed the title
LexOrdering::collapse, avoid cloneLexOrdering::collapse, LexRequirement::collapse avoid clone
| @@ -41,14 +41,9 @@ pub use properties::{ | |||
| /// It will also filter out entries that are ordered if the next entry is; | |||
| /// for instance, `vec![floor(a) Some(ASC), a Some(ASC)]` will be collapsed to | |||
| /// `vec![a Some(ASC)]`. | |||
| #[deprecated(since = "45.0.0", note = "Use LexRequirement::collapse")] | |||
There was a problem hiding this comment.
This function is part of the public API: https://docs.rs/datafusion/latest/datafusion/physical_expr/equivalence/fn.collapse_lex_req.html
| @@ -207,19 +212,6 @@ impl IntoIterator for OrderingEquivalenceClass { | |||
| } | |||
| } | |||
|
|
|||
| /// This function constructs a duplicate-free `LexOrdering` by filtering out | |||
There was a problem hiding this comment.
Despite the function being pub it appears to be in a non pub module, so is not exposed. You can see that it is not present in the docs:
https://docs.rs/datafusion/latest/datafusion/index.html?search=collapse_lex_ordering
(when I marked it deprecated then clippy complained it was dead code)
| let mut output = LexOrdering::default(); | ||
| for item in self { | ||
| if !output.iter().any(|req| req.expr.eq(&item.expr)) { | ||
| output.push(item); |
There was a problem hiding this comment.
In the original code this was item.clone() -- though it is only cloning an Arc so the performance benefit is likely minimal
#26 fix formatting
|
I pushed a44acfd to this PR which had the content of the suggestion from @akurmustafa on alamb#26 |
There was a problem hiding this comment.
Thanks @alamb. The idea makes sense to me. I've just 2 minor question.
| pub fn collapse(self) -> Self { | ||
| let mut output = Vec::<PhysicalSortRequirement>::new(); | ||
| let mut exprs = IndexSet::new(); | ||
| let mut reqs = vec![]; | ||
| for item in self { | ||
| let PhysicalSortRequirement { expr, options: req } = item; | ||
| // new insertion | ||
| if exprs.insert(expr) { | ||
| reqs.push(req); | ||
| } | ||
| } | ||
| debug_assert_eq!(reqs.len(), exprs.len()); | ||
| for (expr, req) in izip!(exprs, reqs) { | ||
| output.push(PhysicalSortRequirement::new(expr, req)); | ||
| } | ||
| LexRequirement::new(output) | ||
| } |
There was a problem hiding this comment.
| pub fn collapse(self) -> Self { | |
| let mut output = Vec::<PhysicalSortRequirement>::new(); | |
| let mut exprs = IndexSet::new(); | |
| let mut reqs = vec![]; | |
| for item in self { | |
| let PhysicalSortRequirement { expr, options: req } = item; | |
| // new insertion | |
| if exprs.insert(expr) { | |
| reqs.push(req); | |
| } | |
| } | |
| debug_assert_eq!(reqs.len(), exprs.len()); | |
| for (expr, req) in izip!(exprs, reqs) { | |
| output.push(PhysicalSortRequirement::new(expr, req)); | |
| } | |
| LexRequirement::new(output) | |
| } | |
| pub fn collapse(self) -> Self { | |
| let mut output = LexRequirement::default(); | |
| for item in self { | |
| if !output.iter().any(|req| req.expr.eq(&item.expr)) { | |
| output.push(item); | |
| } | |
| } | |
| output | |
| } |
Why doesn't this just work, like the other collapse() of LexOrdering?
There was a problem hiding this comment.
I realized alamb#27 now, and makes sense. So, why do we apply the same in LexOrdering::collapse()?
There was a problem hiding this comment.
This is a good point -- I may have been over eager to merge in the PR from @akurmustafa -- I will think about this more carefully and either restore the original version or apply the same type of optimzization to LexOrdering::collapse
There was a problem hiding this comment.
After some more thought I reverted this change and restored it to the original version. My rationale was that I don't (yet) have evidence that this is a bottleneck for processing and thus this seems a bit like a premature optimization
| /// For example, `vec![a Some(ASC), a Some(DESC)]` collapses to `vec![a | ||
| /// Some(ASC)]`. | ||
| /// | ||
| /// It will also filter out entries that are ordered if the next entry is; |
There was a problem hiding this comment.
I couldn't see how that happens 🤔
There was a problem hiding this comment.
I double checked -- and I agree with your assesment so have removed this comment
Which issue does this PR close?
UNIONandORDER BYqueries #13748Rationale for this change
While working to encapsulate the sort order code more, I noticed the free function
collapse_lex_orderingthat consumed aLexOrderingbut wasn't easy to find. It also clones expressions unecessairly.What changes are included in this PR?
LexOrdering::collapseand avoid a cloneLexRequirement::collapsecollapse_lex_reqAre these changes tested?
By exisitng CI
Are there any user-facing changes?
A function is deprecated