alamb commented on code in PR #5863:
URL: https://github.com/apache/arrow-datafusion/pull/5863#discussion_r1158487640
##########
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:
Nice catch. I will fix it (restore to the original)
Update: fixed in a5cc4c25f0659b8d748e87a2ca95e37e626779c8
--
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]
