[GitHub] [arrow-datafusion] ozankabak commented on a diff in pull request #5661: Top down `EnforceSorting`, Extended testbench for `EnforceSorting` rule to prepare for refactors, additional functionality such as pushdowns over unions

["ozankabak (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org