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
alamb
left a comment
alamb
left a comment
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
| 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)
| } | ||
| } | ||
|
|
||
| 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
| 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 |
berkaysynnada
left a comment
berkaysynnada
left a comment
There was a problem hiding this comment.
LGTM, very nice refactor. Thank you @jatin510
|
I have merged with main to resolve conflicts. I will merge this once the tests pass |
|
Thanks again @berkaysynnada and @jatin510 |
* converted LexOrderingRef to &LexOrdering * using LexOrdering::from_ref fn instead of directly cloning it * using as_ref instread of & * using as_ref * removed commented code * updated cargo lock * updated LexRequirementRef to &LexRequirement * fixed clippy issues * fixed taplo error for cargo.toml in physical-expr-common * removed commented code * fixed clippy errors * fixed clippy error * fixes * removed LexOrdering::from_ref instead using clone and created LexOrdering::empty() fn * Update mod.rs --------- Co-authored-by: Berkay Şahin <[email protected]> Co-authored-by: berkaysynnada <[email protected]>
* converted LexOrderingRef to &LexOrdering * using LexOrdering::from_ref fn instead of directly cloning it * using as_ref instread of & * using as_ref * removed commented code * updated cargo lock * updated LexRequirementRef to &LexRequirement * fixed clippy issues * fixed taplo error for cargo.toml in physical-expr-common * removed commented code * fixed clippy errors * fixed clippy error * fixes * removed LexOrdering::from_ref instead using clone and created LexOrdering::empty() fn * Update mod.rs --------- Co-authored-by: Berkay Şahin <[email protected]> Co-authored-by: berkaysynnada <[email protected]> (cherry picked from commit 9005585)
* converted LexOrderingRef to &LexOrdering * using LexOrdering::from_ref fn instead of directly cloning it * using as_ref instread of & * using as_ref * removed commented code * updated cargo lock * updated LexRequirementRef to &LexRequirement * fixed clippy issues * fixed taplo error for cargo.toml in physical-expr-common * removed commented code * fixed clippy errors * fixed clippy error * fixes * removed LexOrdering::from_ref instead using clone and created LexOrdering::empty() fn * Update mod.rs --------- Co-authored-by: Berkay Şahin <[email protected]> Co-authored-by: berkaysynnada <[email protected]> (cherry picked from commit 9005585)
* converted LexOrderingRef to &LexOrdering * using LexOrdering::from_ref fn instead of directly cloning it * using as_ref instread of & * using as_ref * removed commented code * updated cargo lock * updated LexRequirementRef to &LexRequirement * fixed clippy issues * fixed taplo error for cargo.toml in physical-expr-common * removed commented code * fixed clippy errors * fixed clippy error * fixes * removed LexOrdering::from_ref instead using clone and created LexOrdering::empty() fn * Update mod.rs --------- Co-authored-by: Berkay Şahin <[email protected]> Co-authored-by: berkaysynnada <[email protected]> (cherry picked from commit 9005585)
3 participants
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?