Skip to content

Commit 1c3f194

Browse files
findepifindepi
committed
Remove Box from Sort
`expr::Sort` had `Box<Expr>` because Sort was also an expression (via `expr::Expr::Sort`). This has been removed, obsoleting need to use a `Box`.
1 parent 1f74d8e commit 1c3f194

File tree

22 files changed

+47
-60
lines changed

22 files changed

+47
-60
lines changed

‎datafusion/core/src/datasource/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ fn create_ordering(
6363
// Construct PhysicalSortExpr objects from Expr objects:
6464
let mut sort_exprs = vec![];
6565
for sort in exprs {
66-
match sort.expr.as_ref() {
66+
match &sort.expr {
6767
Expr::Column(col) => match expressions::col(&col.name, schema) {
6868
Ok(expr) => {
6969
sort_exprs.push(PhysicalSortExpr {

‎datafusion/core/tests/dataframe/mod.rs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ async fn test_count_wildcard_on_window() -> Result<()> {
184184
WindowFunctionDefinition::AggregateUDF(count_udaf()),
185185
vec![wildcard()],
186186
))
187-
.order_by(vec![Sort::new(Box::new(col("a")), false, true)])
187+
.order_by(vec![Sort::new(col("a"), false, true)])
188188
.window_frame(WindowFrame::new_bounds(
189189
WindowFrameUnits::Range,
190190
WindowFrameBound::Preceding(ScalarValue::UInt32(Some(6))),
@@ -352,7 +352,7 @@ async fn sort_on_unprojected_columns() -> Result<()> {
352352
.unwrap()
353353
.select(vec![col("a")])
354354
.unwrap()
355-
.sort(vec![Sort::new(Box::new(col("b")), false, true)])
355+
.sort(vec![Sort::new(col("b"), false, true)])
356356
.unwrap();
357357
let results = df.collect().await.unwrap();
358358

@@ -396,7 +396,7 @@ async fn sort_on_distinct_columns() -> Result<()> {
396396
.unwrap()
397397
.distinct()
398398
.unwrap()
399-
.sort(vec![Sort::new(Box::new(col("a")), false, true)])
399+
.sort(vec![Sort::new(col("a"), false, true)])
400400
.unwrap();
401401
let results = df.collect().await.unwrap();
402402

@@ -435,7 +435,7 @@ async fn sort_on_distinct_unprojected_columns() -> Result<()> {
435435
.await?
436436
.select(vec![col("a")])?
437437
.distinct()?
438-
.sort(vec![Sort::new(Box::new(col("b")), false, true)])
438+
.sort(vec![Sort::new(col("b"), false, true)])
439439
.unwrap_err();
440440
assert_eq!(err.strip_backtrace(), "Error during planning: For SELECT DISTINCT, ORDER BY expressions b must appear in select list");
441441
Ok(())
@@ -599,8 +599,8 @@ async fn test_grouping_sets() -> Result<()> {
599599
.await?
600600
.aggregate(vec![grouping_set_expr], vec![count(col("a"))])?
601601
.sort(vec![
602-
Sort::new(Box::new(col("a")), false, true),
603-
Sort::new(Box::new(col("b")), false, true),
602+
Sort::new(col("a"), false, true),
603+
Sort::new(col("b"), false, true),
604604
])?;
605605

606606
let results = df.collect().await?;
@@ -640,8 +640,8 @@ async fn test_grouping_sets_count() -> Result<()> {
640640
.await?
641641
.aggregate(vec![grouping_set_expr], vec![count(lit(1))])?
642642
.sort(vec![
643-
Sort::new(Box::new(col("c1")), false, true),
644-
Sort::new(Box::new(col("c2")), false, true),
643+
Sort::new(col("c1"), false, true),
644+
Sort::new(col("c2"), false, true),
645645
])?;
646646

647647
let results = df.collect().await?;
@@ -687,8 +687,8 @@ async fn test_grouping_set_array_agg_with_overflow() -> Result<()> {
687687
],
688688
)?
689689
.sort(vec![
690-
Sort::new(Box::new(col("c1")), false, true),
691-
Sort::new(Box::new(col("c2")), false, true),
690+
Sort::new(col("c1"), false, true),
691+
Sort::new(col("c2"), false, true),
692692
])?;
693693

694694
let results = df.collect().await?;

‎datafusion/core/tests/user_defined/user_defined_plan.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -419,7 +419,7 @@ impl UserDefinedLogicalNodeCore for TopKPlanNode {
419419
}
420420

421421
fn expressions(&self) -> Vec<Expr> {
422-
vec![self.expr.expr.as_ref().clone()]
422+
vec![self.expr.expr.clone()]
423423
}
424424

425425
/// For example: `TopK: k=10`

‎datafusion/expr/src/expr.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -602,7 +602,7 @@ impl TryCast {
602602
#[derive(Clone, PartialEq, Eq, Hash, Debug)]
603603
pub struct Sort {
604604
/// The expression to sort on
605-
pub expr: Box<Expr>,
605+
pub expr: Expr,
606606
/// The direction of the sort
607607
pub asc: bool,
608608
/// Whether to put Nulls before all other data values
@@ -611,7 +611,7 @@ pub struct Sort {
611611

612612
impl Sort {
613613
/// Create a new Sort expression
614-
pub fn new(expr: Box<Expr>, asc: bool, nulls_first: bool) -> Self {
614+
pub fn new(expr: Expr, asc: bool, nulls_first: bool) -> Self {
615615
Self {
616616
expr,
617617
asc,
@@ -1368,7 +1368,7 @@ impl Expr {
13681368
/// let sort_expr = col("foo").sort(true, true); // SORT ASC NULLS_FIRST
13691369
/// ```
13701370
pub fn sort(self, asc: bool, nulls_first: bool) -> Sort {
1371-
Sort::new(Box::new(self), asc, nulls_first)
1371+
Sort::new(self, asc, nulls_first)
13721372
}
13731373

13741374
/// Return `IsTrue(Box(self))`

‎datafusion/expr/src/expr_rewriter/mod.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,8 +125,8 @@ pub fn normalize_sorts(
125125
.into_iter()
126126
.map(|e| {
127127
let sort = e.into();
128-
normalize_col(*sort.expr, plan)
129-
.map(|expr| Sort::new(Box::new(expr), sort.asc, sort.nulls_first))
128+
normalize_col(sort.expr, plan)
129+
.map(|expr| Sort::new(expr, sort.asc, sort.nulls_first))
130130
})
131131
.collect()
132132
}

‎datafusion/expr/src/expr_rewriter/order_by.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ pub fn rewrite_sort_cols_by_aggs(
3535
.map(|e| {
3636
let sort = e.into();
3737
Ok(Sort::new(
38-
Box::new(rewrite_sort_col_by_aggs(*sort.expr, plan)?),
38+
rewrite_sort_col_by_aggs(sort.expr, plan)?,
3939
sort.asc,
4040
sort.nulls_first,
4141
))

‎datafusion/expr/src/logical_plan/builder.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1723,8 +1723,8 @@ mod tests {
17231723
let plan =
17241724
table_scan(Some("employee_csv"), &employee_schema(), Some(vec![3, 4]))?
17251725
.sort(vec![
1726-
expr::Sort::new(Box::new(col("state")), true, true),
1727-
expr::Sort::new(Box::new(col("salary")), false, false),
1726+
expr::Sort::new(col("state"), true, true),
1727+
expr::Sort::new(col("salary"), false, false),
17281728
])?
17291729
.build()?;
17301730

@@ -2150,8 +2150,8 @@ mod tests {
21502150
let plan =
21512151
table_scan(Some("employee_csv"), &employee_schema(), Some(vec![3, 4]))?
21522152
.sort(vec![
2153-
expr::Sort::new(Box::new(col("state")), true, true),
2154-
expr::Sort::new(Box::new(col("salary")), false, false),
2153+
expr::Sort::new(col("state"), true, true),
2154+
expr::Sort::new(col("salary"), false, false),
21552155
])?
21562156
.build()?;
21572157

‎datafusion/expr/src/logical_plan/plan.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2616,7 +2616,7 @@ impl DistinctOn {
26162616
// Check that the left-most sort expressions are the same as the `ON` expressions.
26172617
let mut matched = true;
26182618
for (on, sort) in self.on_expr.iter().zip(sort_expr.iter()) {
2619-
if on != &*sort.expr {
2619+
if on != &sort.expr {
26202620
matched = false;
26212621
break;
26222622
}

‎datafusion/expr/src/logical_plan/tree_node.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -509,7 +509,7 @@ impl LogicalPlan {
509509
})) => on_expr
510510
.iter()
511511
.chain(select_expr.iter())
512-
.chain(sort_expr.iter().flatten().map(|sort| &*sort.expr))
512+
.chain(sort_expr.iter().flatten().map(|sort| &sort.expr))
513513
.apply_until_stop(f),
514514
// plans without expressions
515515
LogicalPlan::EmptyRelation(_)

‎datafusion/expr/src/tree_node.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ impl TreeNode for Expr {
9797
expr_vec.push(f.as_ref());
9898
}
9999
if let Some(order_by) = order_by {
100-
expr_vec.extend(order_by.iter().map(|sort| sort.expr.as_ref()));
100+
expr_vec.extend(order_by.iter().map(|sort| &sort.expr));
101101
}
102102
expr_vec
103103
}
@@ -109,7 +109,7 @@ impl TreeNode for Expr {
109109
}) => {
110110
let mut expr_vec = args.iter().collect::<Vec<_>>();
111111
expr_vec.extend(partition_by);
112-
expr_vec.extend(order_by.iter().map(|sort| sort.expr.as_ref()));
112+
expr_vec.extend(order_by.iter().map(|sort| &sort.expr));
113113
expr_vec
114114
}
115115
Expr::InList(InList { expr, list, .. }) => {
@@ -395,7 +395,7 @@ pub fn transform_sort_vec<F: FnMut(Expr) -> Result<Transformed<Expr>>>(
395395
) -> Result<Transformed<Vec<Sort>>> {
396396
Ok(sorts
397397
.iter()
398-
.map(|sort| (*sort.expr).clone())
398+
.map(|sort| sort.expr.clone())
399399
.map_until_stop_and_collect(&mut f)?
400400
.update_data(|transformed_exprs| {
401401
replace_sort_expressions(sorts, transformed_exprs)
@@ -420,7 +420,7 @@ pub fn replace_sort_expressions(sorts: Vec<Sort>, new_expr: Vec<Expr>) -> Vec<So
420420

421421
pub fn replace_sort_expression(sort: Sort, new_expr: Expr) -> Sort {
422422
Sort {
423-
expr: Box::new(new_expr),
423+
expr: new_expr,
424424
..sort
425425
}
426426
}

‎datafusion/expr/src/utils.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1401,9 +1401,9 @@ mod tests {
14011401

14021402
#[test]
14031403
fn test_group_window_expr_by_sort_keys() -> Result<()> {
1404-
let age_asc = expr::Sort::new(Box::new(col("age")), true, true);
1405-
let name_desc = expr::Sort::new(Box::new(col("name")), false, true);
1406-
let created_at_desc = expr::Sort::new(Box::new(col("created_at")), false, true);
1404+
let age_asc = expr::Sort::new(col("age"), true, true);
1405+
let name_desc = expr::Sort::new(col("name"), false, true);
1406+
let created_at_desc = expr::Sort::new(col("created_at"), false, true);
14071407
let max1 = Expr::WindowFunction(expr::WindowFunction::new(
14081408
WindowFunctionDefinition::AggregateUDF(max_udaf()),
14091409
vec![col("name")],
@@ -1463,12 +1463,12 @@ mod tests {
14631463
for nulls_first_ in nulls_first_or_last {
14641464
let order_by = &[
14651465
Sort {
1466-
expr: Box::new(col("age")),
1466+
expr: col("age"),
14671467
asc: asc_,
14681468
nulls_first: nulls_first_,
14691469
},
14701470
Sort {
1471-
expr: Box::new(col("name")),
1471+
expr: col("name"),
14721472
asc: asc_,
14731473
nulls_first: nulls_first_,
14741474
},
@@ -1477,23 +1477,23 @@ mod tests {
14771477
let expected = vec![
14781478
(
14791479
Sort {
1480-
expr: Box::new(col("age")),
1480+
expr: col("age"),
14811481
asc: asc_,
14821482
nulls_first: nulls_first_,
14831483
},
14841484
true,
14851485
),
14861486
(
14871487
Sort {
1488-
expr: Box::new(col("name")),
1488+
expr: col("name"),
14891489
asc: asc_,
14901490
nulls_first: nulls_first_,
14911491
},
14921492
true,
14931493
),
14941494
(
14951495
Sort {
1496-
expr: Box::new(col("created_at")),
1496+
expr: col("created_at"),
14971497
asc: true,
14981498
nulls_first: false,
14991499
},

‎datafusion/optimizer/src/analyzer/count_wildcard_rule.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,7 @@ mod tests {
229229
WindowFunctionDefinition::AggregateUDF(count_udaf()),
230230
vec![wildcard()],
231231
))
232-
.order_by(vec![Sort::new(Box::new(col("a")), false, true)])
232+
.order_by(vec![Sort::new(col("a"), false, true)])
233233
.window_frame(WindowFrame::new_bounds(
234234
WindowFrameUnits::Range,
235235
WindowFrameBound::Preceding(ScalarValue::UInt32(Some(6))),

‎datafusion/optimizer/src/common_subexpr_eliminate.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -328,8 +328,7 @@ impl CommonSubexprEliminate {
328328
) -> Result<Transformed<LogicalPlan>> {
329329
let Sort { expr, input, fetch } = sort;
330330
let input = Arc::unwrap_or_clone(input);
331-
let sort_expressions =
332-
expr.iter().map(|sort| sort.expr.as_ref().clone()).collect();
331+
let sort_expressions = expr.iter().map(|sort| sort.expr.clone()).collect();
333332
let new_sort = self
334333
.try_unary_plan(sort_expressions, input, config)?
335334
.update_data(|(new_expr, new_input)| {

‎datafusion/optimizer/src/eliminate_duplicated_expr.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ impl OptimizerRule for EliminateDuplicatedExpr {
5555
IndexMap::default();
5656
for s in &sort.expr {
5757
first_sort_by_expr
58-
.entry(s.expr.as_ref().clone())
58+
.entry(s.expr.clone())
5959
.or_insert(s.clone());
6060
}
6161
let unique_exprs: Vec<SortExpr> =

‎datafusion/proto/src/logical_plan/from_proto.rs

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -645,12 +645,7 @@ pub fn parse_sort(
645645
codec: &dyn LogicalExtensionCodec,
646646
) -> Result<Sort, Error> {
647647
Ok(Sort::new(
648-
Box::new(parse_required_expr(
649-
sort.expr.as_ref(),
650-
registry,
651-
"expr",
652-
codec,
653-
)?),
648+
parse_required_expr(sort.expr.as_ref(), registry, "expr", codec)?,
654649
sort.asc,
655650
sort.nulls_first,
656651
))

‎datafusion/proto/src/logical_plan/to_proto.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -637,7 +637,7 @@ where
637637
nulls_first,
638638
} = sort;
639639
Ok(protobuf::SortExprNode {
640-
expr: Some(serialize_expr(expr.as_ref(), codec)?),
640+
expr: Some(serialize_expr(expr, codec)?),
641641
asc: *asc,
642642
nulls_first: *nulls_first,
643643
})

‎datafusion/sql/src/expr/function.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
283283
let func_deps = schema.functional_dependencies();
284284
// Find whether ties are possible in the given ordering
285285
let is_ordering_strict = order_by.iter().find_map(|orderby_expr| {
286-
if let Expr::Column(col) = orderby_expr.expr.as_ref() {
286+
if let Expr::Column(col) = &orderby_expr.expr {
287287
let idx = schema.index_of_column(col).ok()?;
288288
return if func_deps.iter().any(|dep| {
289289
dep.source_indices == vec![idx] && dep.mode == Dependency::Single

‎datafusion/sql/src/expr/order_by.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
100100
};
101101
let asc = asc.unwrap_or(true);
102102
expr_vec.push(Sort::new(
103-
Box::new(expr),
103+
expr,
104104
asc,
105105
// when asc is true, by default nulls last to be consistent with postgres
106106
// postgres rule: https://www.postgresql.org/docs/current/queries-order.html

‎datafusion/sql/src/unparser/expr.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1761,7 +1761,7 @@ mod tests {
17611761
fun: WindowFunctionDefinition::AggregateUDF(count_udaf()),
17621762
args: vec![wildcard()],
17631763
partition_by: vec![],
1764-
order_by: vec![Sort::new(Box::new(col("a")), false, true)],
1764+
order_by: vec![Sort::new(col("a"), false, true)],
17651765
window_frame: WindowFrame::new_bounds(
17661766
datafusion_expr::WindowFrameUnits::Range,
17671767
datafusion_expr::WindowFrameBound::Preceding(

‎datafusion/sql/src/unparser/rewrite.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ pub(super) fn rewrite_plan_for_sort_on_non_projected_fields(
158158

159159
let mut collects = p.expr.clone();
160160
for sort in &sort.expr {
161-
collects.push(sort.expr.as_ref().clone());
161+
collects.push(sort.expr.clone());
162162
}
163163

164164
// Compare outer collects Expr::to_string with inner collected transformed values

‎datafusion/substrait/src/logical_plan/consumer.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -936,7 +936,7 @@ pub async fn from_substrait_sorts(
936936
};
937937
let (asc, nulls_first) = asc_nullfirst.unwrap();
938938
sorts.push(Sort {
939-
expr: Box::new(expr),
939+
expr,
940940
asc,
941941
nulls_first,
942942
});

‎datafusion/substrait/src/logical_plan/producer.rs

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
// under the License.
1717

1818
use itertools::Itertools;
19-
use std::ops::Deref;
2019
use std::sync::Arc;
2120

2221
use arrow_buffer::ToByteSlice;
@@ -819,13 +818,7 @@ fn to_substrait_sort_field(
819818
(false, false) => SortDirection::DescNullsLast,
820819
};
821820
Ok(SortField {
822-
expr: Some(to_substrait_rex(
823-
ctx,
824-
sort.expr.deref(),
825-
schema,
826-
0,
827-
extensions,
828-
)?),
821+
expr: Some(to_substrait_rex(ctx, &sort.expr, schema, 0, extensions)?),
829822
sort_kind: Some(SortKind::Direction(sort_kind.into())),
830823
})
831824
}

0 commit comments

Comments
 (0)