addressed comments

Author: xiedeyantu

datafusion/common/src/functional_dependencies.rs

@@ -595,14 +595,17 @@ pub fn get_required_group_by_exprs_indices(
 pub fn get_required_sort_exprs_indices(
     schema: &DFSchema,
     sort_expr_names: &[String],
-) -> Option<Vec<usize>> {
+) -> Vec<usize> {
     let dependencies = schema.functional_dependencies();
     let field_names = schema.field_names();
 
     let mut known_field_indices = HashSet::new();
     let mut required_sort_expr_indices = Vec::new();
 
     for (sort_expr_idx, sort_expr_name) in sort_expr_names.iter().enumerate() {
+        // If the sort expression doesn't correspond to a known schema field
+        // (e.g. a computed expression), we can't reason about it via functional
+        // dependencies, so conservatively keep it.
         let Some(field_idx) = field_names
             .iter()
             .position(|field_name| field_name == sort_expr_name)
@@ -611,6 +614,10 @@ pub fn get_required_sort_exprs_indices(
             continue;
         };
 
+        // A sort expression is removable if its value is functionally determined
+        // by fields that already appear earlier in the sort order: if the earlier
+        // fields are fixed, this one's value is fixed too, so it adds no ordering
+        // information.
         let removable = dependencies.deps.iter().any(|dependency| {
             dependency.target_indices.contains(&field_idx)
                 && dependency
@@ -627,7 +634,7 @@ pub fn get_required_sort_exprs_indices(
         required_sort_expr_indices.push(sort_expr_idx);
     }
 
-    Some(required_sort_expr_indices)
+    required_sort_expr_indices
 }
 
 /// Updates entries inside the `entries` vector with their corresponding

datafusion/optimizer/src/eliminate_duplicated_expr.rs

@@ -20,7 +20,7 @@
 use crate::optimizer::ApplyOrder;
 use crate::{OptimizerConfig, OptimizerRule};
 use datafusion_common::tree_node::Transformed;
-use datafusion_common::{Result, get_required_sort_exprs_indices};
+use datafusion_common::{Result, get_required_sort_exprs_indices, internal_err};
 use datafusion_expr::logical_plan::LogicalPlan;
 use datafusion_expr::{Aggregate, Expr, Sort, SortExpr};
 use std::hash::{Hash, Hasher};
@@ -85,12 +85,13 @@ impl OptimizerRule for EliminateDuplicatedExpr {
                     &sort_expr_names,
                 );
 
-                let unique_exprs = match required_indices {
-                    Some(indices) if indices.len() < unique_exprs.len() => indices
+                let unique_exprs = if required_indices.len() < unique_exprs.len() {
+                    required_indices
                         .into_iter()
                         .map(|idx| unique_exprs[idx].clone())
-                        .collect(),
-                    _ => unique_exprs,
+                        .collect()
+                } else {
+                    unique_exprs
                 };
 
                 let transformed = if len != unique_exprs.len() {
@@ -99,10 +100,11 @@ impl OptimizerRule for EliminateDuplicatedExpr {
                     Transformed::no
                 };
 
-                assert!(
-                    !unique_exprs.is_empty(),
-                    "FD pruning unexpectedly removed all ORDER BY expressions"
-                );
+                if unique_exprs.is_empty() {
+                    return internal_err!(
+                        "FD pruning unexpectedly removed all ORDER BY expressions"
+                    );
+                }
 
                 Ok(transformed(LogicalPlan::Sort(Sort {
                     expr: unique_exprs,