alamb commented on code in PR #15841:
URL: https://github.com/apache/datafusion/pull/15841#discussion_r2073508627
##########
datafusion/physical-optimizer/src/enforce_sorting/mod.rs:
##########
@@ -502,10 +502,11 @@ fn analyze_immediate_sort_removal(
let sort_input = sort_exec.input();
// If this sort is unnecessary, we should remove it:
if sort_input.equivalence_properties().ordering_satisfy(
- sort_exec
+ &sort_exec
.properties()
.output_ordering()
- .unwrap_or(LexOrdering::empty()),
+ .cloned()
Review Comment:
this causes a new copy (`cloned()`) and allocations where the previous
version does not (it makes a LexOrdering::empty() which has an empty `Vec` and
thus has no allocations and is cheap to create). Thus I think we should revert
this change
##########
datafusion/physical-optimizer/src/enforce_sorting/replace_with_order_preserving_variants.rs:
##########
@@ -27,8 +27,10 @@ use crate::utils::{
use datafusion_common::config::ConfigOptions;
use datafusion_common::tree_node::Transformed;
-use datafusion_common::{internal_err, Result};
-use datafusion_physical_expr_common::sort_expr::LexOrdering;
+use datafusion_physical_plan::internal_err;
+
+use datafusion_common::Result;
+// use datafusion_physical_expr_common::sort_expr::LexOrdering;
Review Comment:
can we please remove this comment
##########
datafusion/physical-optimizer/src/update_aggr_exprs.rs:
##########
@@ -159,7 +159,7 @@ fn try_convert_aggregate_if_better(
aggr_exprs
.into_iter()
.map(|aggr_expr| {
- let aggr_sort_exprs =
aggr_expr.order_bys().unwrap_or(LexOrdering::empty());
+ let aggr_sort_exprs =
&aggr_expr.order_bys().cloned().unwrap_or_default();
Review Comment:
This cloned is also a regression in my mind and should be reverted
##########
datafusion/physical-optimizer/src/enforce_sorting/replace_with_order_preserving_variants.rs:
##########
@@ -283,10 +285,11 @@ pub fn replace_with_order_preserving_variants(
.plan
.equivalence_properties()
.ordering_satisfy(
- requirements
+ &requirements
.plan
.output_ordering()
- .unwrap_or(LexOrdering::empty()),
+ .cloned()
Review Comment:
same thing here, this cloned is worrysome to me
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
