ozankabak commented on code in PR #5661:
URL: https://github.com/apache/arrow-datafusion/pull/5661#discussion_r1148422676
##########
datafusion/physical-expr/src/sort_expr.rs:
##########
@@ -69,4 +62,70 @@ impl PhysicalSortExpr {
options: Some(self.options),
})
}
+
+ pub fn satisfy(&self, requirement: &PhysicalSortRequirement) -> bool {
+ self.expr.eq(&requirement.expr)
+ && requirement
+ .options
+ .map_or(true, |opts| self.options == opts)
+ }
+}
+
+/// Represents sort requirement associated with a plan
+#[derive(Clone, Debug)]
+pub struct PhysicalSortRequirement {
+ /// Physical expression representing the column to sort
Review Comment:
The main difference between a `PhysicalSortExpr` and a
`PhysicalSortRequirement` is that `SortOptions` is optional in the latter, but
not the former.
The former carries the actual ordering information in the plan, while the
latter is used to specify ordering requirements. A natural question here would
be: Why do we need two structs? Can't we just use `PhysicalSortExpr` in both
contexts? That is indeed what we used to do, but our collaboration with
@mingmwang revealed that there are cases where a requirement specifies the
sorting columns, but doesn't care about the ascending/descending distinction.
Therefore, he proposed the `PhysicalSortRequirement` in one of his commits. We
agreed with the rationale and adopted it in this work.
I hope that helps!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
