[GitHub] [arrow-datafusion] ozankabak commented on a diff in pull request #5863: Encapsulate `PhysicalSortRequrement` and add more doc comment

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

ozankabak commented on code in PR #5863:
URL: https://github.com/apache/arrow-datafusion/pull/5863#discussion_r1157570060


##########
datafusion/physical-expr/src/sort_expr.rs:
##########
@@ -107,19 +147,94 @@ impl std::fmt::Display for PhysicalSortRequirement {
 }
 
 impl PhysicalSortRequirement {
+    /// Creates a new `exact` requirement, which must match the
+    /// required options and expression. See
+    /// [`PhysicalSortRequirement`] for examples.
+    pub fn new_exact(expr: Arc<dyn PhysicalExpr>, options: SortOptions) -> Self {
+        Self {
+            expr,
+            options: Some(options),
+        }
+    }
+
+    /// Creates a new `expr_only` requirement, which must match the
+    /// required expression. See [`PhysicalSortRequirement`] for
+    /// examples.
+    pub fn new_expr_only(expr: Arc<dyn PhysicalExpr>) -> Self {
+        Self {
+            expr,
+            options: None,
+        }
+    }
+
+    /// 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 {

Review Comment:
   I'd suggest `unwrap_or_else` to explicitly avoid constructing a potentially unused object. The compiler will likely take care of this, but I find making intent visible in code structure.



##########
datafusion/physical-expr/src/sort_expr.rs:
##########
@@ -107,19 +147,94 @@ impl std::fmt::Display for PhysicalSortRequirement {
 }
 
 impl PhysicalSortRequirement {
+    /// Creates a new `exact` requirement, which must match the
+    /// required options and expression. See
+    /// [`PhysicalSortRequirement`] for examples.
+    pub fn new_exact(expr: Arc<dyn PhysicalExpr>, options: SortOptions) -> Self {
+        Self {
+            expr,
+            options: Some(options),
+        }
+    }
+
+    /// Creates a new `expr_only` requirement, which must match the
+    /// required expression. See [`PhysicalSortRequirement`] for
+    /// examples.
+    pub fn new_expr_only(expr: Arc<dyn PhysicalExpr>) -> Self {
+        Self {
+            expr,
+            options: None,
+        }
+    }

Review Comment:
   You could have a single `new` and the caller can pass the second argument as `None` if they don't want to constrain sort options. I get the sense that you have two `new`s because you think it is more readable? 



##########
datafusion/physical-expr/src/sort_expr.rs:
##########
@@ -75,21 +79,57 @@ impl PhysicalSortExpr {
 }
 
 /// Represents sort requirement associated with a plan
+///
+/// If the requirement is *exact*
+/// ([`PhysicalSortRequirement::new_exact`]), then the sort
+/// requirement will only be satisfied if it matches both the
+/// expression *and* the sort options.
+///
+/// If the requirement is *`expr_only`
+/// ([`PhysicalSortRequirement::new_expr_only`]) then only the expr
+/// must match, to satisfy the requirement.
+///
+/// # Examples
+///
+/// Given an `exact` sort requirement of (`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)
+///
+/// Given an `expr_only` sort requirement of (`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
-    pub expr: Arc<dyn PhysicalExpr>,
+    expr: Arc<dyn PhysicalExpr>,
     /// Option to specify how the given column should be sorted.
-    /// If unspecified, there is no constraint on sort options.
-    pub options: Option<SortOptions>,
+    /// If unspecified, there are constraints on sort options.

Review Comment:
   This comment seems incorrect, did you mean to start with "If specified, ..."?



-- 
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