Deprecate LexOrderingRef and LexRequirementRef
#13233
Conversation
github-actions
bot
added
physical-expr
|
@alamb am i going in right directions ? |
jatin510
force-pushed
the
feature/13220-deprecate-LexOrderingRef-and-LexRequirementRef
branch
from
0c2e077
to
a9023bf
Compare
jatin510
changed the title
LexOrderingRef and LexRequirementRef
There was a problem hiding this comment.
@alamb am i going in right directions ?
as this PR changes lots of files ? 😅
Yes, THANK YOU @jatin510 -- I think this is looking really nice. I left a few comments about how to improve this further, but overall it is definitely in the right direction
I agree it is a large change, but this PR (along with the previous one) removes one of the major "rough edges" in the LexOrdering API in my opinon
FYI @akurmustafa and @berkaysynnada
If you are willing to polish this a little more that would be great. I also think we could do the polishing as a follow on PR if you prefer
| pub fn capacity(&self) -> usize { | ||
| self.inner.capacity() | ||
| } | ||
|
|
||
| pub fn from_ref(lex_ordering_ref: &LexOrdering) -> Self { |
There was a problem hiding this comment.
I think this method could be removed as it is the same as clone()
| self | ||
| } | ||
|
|
||
| /// Converts a `LexRequirement` into a `LexOrdering`. |
There was a problem hiding this comment.
I also included this change in #13222 (mostly as a note that this will be a logical conflict)
| @@ -545,6 +596,16 @@ impl IntoIterator for LexRequirement { | |||
| } | |||
| } | |||
|
|
|||
| impl<'a> IntoIterator for &'a LexOrdering { | |||
| Self { | ||
| streams: vec![], | ||
| schema: None, | ||
| expressions: EMPTY_ORDER.get_or_init(LexOrdering::default), |
There was a problem hiding this comment.
👍
What do you think about moving this EMPTY_ORDER to LexOrdering?
Something like
impl LexOrdering {
...
/// Return an empty LexOrdering (no expressions)
pub empty() -> &'static LexOrdering {
EMPTY_ORDER.get_or_init(LexOrdering::default),
}
}That would avoid needing to copy LexOrderings quite as much (I'll comment inline
| @@ -1553,7 +1552,7 @@ mod tests { | |||
| Arc::new(BuiltInWindowExpr::new( | |||
| last_value_func, | |||
| &[], | |||
| LexOrderingRef::default(), | |||
| LexOrdering::default().as_ref(), | |||
There was a problem hiding this comment.
I don't think the as_ref() is needed here (it just makes another copy)
There was a problem hiding this comment.
using
&LexOrdering::default()
| .output_ordering() | ||
| .cloned() | ||
| .unwrap_or_default(), |
There was a problem hiding this comment.
I think if we had LexOrdering::empty() as explained below, we could avoid this clone like this
&sort_exec
.properties()
.output_ordering()
.unwrap_or(LexOrdering::empty()),There are similar things below too
|
Thanks ! |
|
FYI @berkaysynnada |
|
I have merged with main to resolve conflicts. I will merge this once the tests pass |
|
Thanks again @berkaysynnada and @jatin510 |
Which issue does this PR close?
Closes #13220.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?