Encapsulate PhysicalSortRequrement and add more doc comments
#5863
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,9 +63,13 @@ impl PhysicalSortExpr { | |
| }) | ||
| } | ||
|
|
||
| /// Check whether sort expression satisfies [`PhysicalSortRequirement`]. | ||
| /// | ||
| /// If sort options is Some in `PhysicalSortRequirement`, `expr` | ||
| /// and `options` field are compared for equality. | ||
| /// | ||
| /// If sort options is None in `PhysicalSortRequirement`, only | ||
| /// `expr` is compared for equality. | ||
| pub fn satisfy(&self, requirement: &PhysicalSortRequirement) -> bool { | ||
| self.expr.eq(&requirement.expr) | ||
| && requirement | ||
|
|
@@ -75,21 +79,42 @@ impl PhysicalSortExpr { | |
| } | ||
|
|
||
| /// Represents sort requirement associated with a plan | ||
| /// | ||
| /// If the requirement incudes [`SortOptions`] then both the | ||
| /// expression *and* the sort options must match. | ||
| /// | ||
| /// If the requirement does not include [`SortOptions`]) then only the | ||
| /// expressions must match. | ||
| /// | ||
| /// # Examples | ||
| /// | ||
| /// With sort options (`A`, `DESC NULLS FIRST`): | ||
| /// * `ORDER BY A DESC NULLS FIRST` matches | ||
| /// * `ORDER BY A ASC NULLS FIRST` does not match (`ASC` vs `DESC`) | ||
| /// * `ORDER BY B DESC NULLS FIRST` does not match (different expr) | ||
| /// | ||
| /// Without sort options (`A`, None): | ||
| /// * `ORDER BY A DESC NULLS FIRST` matches | ||
| /// * `ORDER BY A ASC NULLS FIRST` matches (`ASC` and `NULL` options ignored) | ||
| /// * `ORDER BY B DESC NULLS FIRST` does not match (different expr) | ||
| #[derive(Clone, Debug)] | ||
| pub struct PhysicalSortRequirement { | ||
| /// Physical expression representing the column to sort | ||
| expr: Arc<dyn PhysicalExpr>, | ||
| /// Option to specify how the given column should be sorted. | ||
| /// If unspecified, there are no constraints on sort options. | ||
| options: Option<SortOptions>, | ||
| } | ||
|
|
||
| impl From<PhysicalSortRequirement> for PhysicalSortExpr { | ||
| fn from(value: PhysicalSortRequirement) -> Self { | ||
| value.into_sort_expr() | ||
| } | ||
| } | ||
|
|
||
| impl From<PhysicalSortExpr> for PhysicalSortRequirement { | ||
| fn from(value: PhysicalSortExpr) -> Self { | ||
| PhysicalSortRequirement::new(value.expr, Some(value.options)) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -107,19 +132,88 @@ impl std::fmt::Display for PhysicalSortRequirement { | |
| } | ||
|
|
||
| impl PhysicalSortRequirement { | ||
| /// Creates a new requirement. | ||
| /// | ||
| /// If `options` is `Some(..)`, creates an `exact` requirement, | ||
| /// which must match both `options` and `expr`. | ||
| /// | ||
| /// If `options` is `None`, Creates a new `expr_only` requirement, | ||
| /// which must match only `expr`. | ||
| /// | ||
| /// See [`PhysicalSortRequirement`] for examples. | ||
| pub fn new(expr: Arc<dyn PhysicalExpr>, options: Option<SortOptions>) -> Self { | ||
| Self { expr, options } | ||
| } | ||
|
|
||
| /// Replace the required expression for this requirement with the new one | ||
| pub fn with_expr(mut self, expr: Arc<dyn PhysicalExpr>) -> Self { | ||
| self.expr = expr; | ||
| self | ||
| } | ||
|
|
||
| /// Converts the `PhysicalSortRequirement` to `PhysicalSortExpr`. | ||
| /// If required ordering is `None` for an entry, the default | ||
| /// ordering `ASC, NULLS LAST` is used. | ||
| /// | ||
| /// The default is picked to be consistent with | ||
| /// PostgreSQL: <https://www.postgresql.org/docs/current/queries-order.html> | ||
| pub fn into_sort_expr(self) -> PhysicalSortExpr { | ||
| let Self { expr, options } = self; | ||
|
|
||
| let options = options.unwrap_or(SortOptions { | ||
| descending: false, | ||
| nulls_first: false, | ||
| }); | ||
| PhysicalSortExpr { expr, options } | ||
| } | ||
|
|
||
| /// Returns whether this requirement is equal or more specific than `other`. | ||
| pub fn compatible(&self, other: &PhysicalSortRequirement) -> bool { | ||
| self.expr.eq(&other.expr) | ||
| && other.options.map_or(true, |other_opts| { | ||
| self.options.map_or(false, |opts| opts == other_opts) | ||
| }) | ||
| } | ||
|
|
||
| /// Returns [`PhysicalSortRequirement`] that requires the exact | ||
| /// sort of the [`PhysicalSortExpr`]s in `ordering` | ||
| /// | ||
| /// This method takes `&'a PhysicalSortExpr` to make it easy to | ||
| /// use implementing [`ExecutionPlan::required_input_ordering`]. | ||
| pub fn from_sort_exprs<'a>( | ||
| ordering: impl IntoIterator<Item = &'a PhysicalSortExpr>, | ||
| ) -> Vec<PhysicalSortRequirement> { | ||
| ordering | ||
| .into_iter() | ||
| .cloned() | ||
| .map(PhysicalSortRequirement::from) | ||
| .collect() | ||
| } | ||
|
|
||
| /// Converts an iterator of [`PhysicalSortRequirement`] into a Vec | ||
| /// of [`PhysicalSortExpr`]s. | ||
| /// | ||
| /// This function converts `PhysicalSortRequirement` to `PhysicalSortExpr` | ||
| /// for each entry in the input. If required ordering is None for an entry | ||
| /// default ordering `ASC, NULLS LAST` if given (see [`into_sort_expr`]) | ||
| pub fn to_sort_exprs( | ||
| requirements: impl IntoIterator<Item = PhysicalSortRequirement>, | ||
| ) -> Vec<PhysicalSortExpr> { | ||
|
Comment on lines
+199
to
+201
Contributor
There was a problem hiding this comment. I think we can use pub fn to_sort_exprs<'a>(
requirements: impl IntoIterator<Item = &'a PhysicalSortRequirement>,
) -> Vec<PhysicalSortExpr> as signature for
Contributor
Author
There was a problem hiding this comment. My rationale for taking Specifically in // Make sure we preserve the ordering requirements:
update_child_to_remove_unnecessary_sort(child, sort_onwards, &plan)?;
let sort_expr =
PhysicalSortRequirement::to_sort_exprs(required_ordering);
add_sort_above(child, sort_expr)?;
if is_sort(child) {This signature can avoid a clone. If the signature looks like pub fn to_sort_exprs<'a>(
requirements: impl IntoIterator<Item = &'a PhysicalSortRequirement>,
) -> Vec<PhysicalSortExpr> I think it would require always Of course, we are talking about cloning an
Contributor
There was a problem hiding this comment. I have a feeling there may be a way to do this with a
Contributor
There was a problem hiding this comment. @alamb, it seems like using pub fn to_sort_exprs<T: Borrow<PhysicalSortRequirement>>(
requirements: impl IntoIterator<Item = T>,
) -> Vec<PhysicalSortExpr> {
requirements
.into_iter()
.map(|e| PhysicalSortExpr::from(e.borrow()))
.collect()
}
pub fn from_sort_exprs<T: Borrow<PhysicalSortExpr>>(
ordering: impl IntoIterator<Item = T>,
) -> Vec<PhysicalSortRequirement> {
ordering
.into_iter()
.map(|e| PhysicalSortRequirement::from(e.borrow()))
.collect()
}lets us get rid of a bunch of I created #11 on your repo so you can easily check it out and incorporate (assuming I'm not overlooking something and this is indeed zero cost obviously).
Contributor
Author
There was a problem hiding this comment. I guess my point was that I think this call: PhysicalSortRequirement::from(e.borrow()Invokes this Which has a I have removed the
Contributor
There was a problem hiding this comment. I see. I thought you wanted to make Anyway I got curious and tried to verify whether the For posterity, had we wanted to go this route, the smart pub fn to_sort_exprs<T>(
requirements: impl IntoIterator<Item = T>,
) -> Vec<PhysicalSortExpr>
where
PhysicalSortExpr: From<T>,
{
requirements
.into_iter()
.map(|e| PhysicalSortExpr::from(e))
.collect()
}which would route to the right
Contributor
Author
There was a problem hiding this comment. Very cool -- I did not know that. If you think that is a worthwhile change, I would be happy to make a follow on PR
Contributor
There was a problem hiding this comment. I think it'd be a good change, I can already see some use cases for it in some upcoming code (e.g. streaming group bys). I updated the mini-PR I created for experimentation so you can get the changes.
Contributor
Author
|
||
| requirements | ||
| .into_iter() | ||
| .map(PhysicalSortExpr::from) | ||
| .collect() | ||
| } | ||
|
|
||
| /// Returns the expr for this requirement | ||
| pub fn expr(&self) -> &Arc<dyn PhysicalExpr> { | ||
| &self.expr | ||
| } | ||
|
|
||
| /// Returns the required options, for this requirement | ||
| pub fn options(&self) -> Option<SortOptions> { | ||
| self.options | ||
| } | ||
| } | ||
|
|
||
| /// Returns the SQL string representation of the given [SortOptions] object. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest
unwrap_or_elseto explicitly avoid constructing a potentially unused object. The compiler will likely take care of this, but I find making intent visible in code structure is good practice.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed -- I agree with you. I originally had
unwrap_or_elsefor precisely the reasons you mentioned, butclippytold me to useunwrap_orinstead 😭