Skip to content

Commit 50dc83a

Browse files
ViggoCViggoC
authored
Convert Option<Vec<sort expression>> to Vec<sort expression> (apache#16615)
* Convert Option<Vec<sort expression>> to Vec<sort expression> * clippy * fix comment * fix doc * change back to Expr * remove redundant check
1 parent 06e5bbe commit 50dc83a

23 files changed

Lines changed: 97 additions & 128 deletions

File tree

‎datafusion/core/src/physical_planner.rs‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1651,14 +1651,11 @@ pub fn create_aggregate_expr_with_name_and_maybe_filter(
16511651
== NullTreatment::IgnoreNulls;
16521652

16531653
let (agg_expr, filter, order_bys) = {
1654-
let order_bys = match order_by {
1655-
Some(exprs) => create_physical_sort_exprs(
1656-
exprs,
1657-
logical_input_schema,
1658-
execution_props,
1659-
)?,
1660-
None => vec![],
1661-
};
1654+
let order_bys = create_physical_sort_exprs(
1655+
order_by,
1656+
logical_input_schema,
1657+
execution_props,
1658+
)?;
16621659

16631660
let agg_expr =
16641661
AggregateExprBuilder::new(func.to_owned(), physical_args.to_vec())

‎datafusion/core/tests/execution/logical_plan.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ async fn count_only_nulls() -> Result<()> {
6868
args: vec![input_col_ref],
6969
distinct: false,
7070
filter: None,
71-
order_by: None,
71+
order_by: vec![],
7272
null_treatment: None,
7373
},
7474
})],

‎datafusion/expr/src/expr.rs‎

Lines changed: 17 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ use crate::expr_fn::binary_expr;
2828
use crate::function::WindowFunctionSimplification;
2929
use crate::logical_plan::Subquery;
3030
use crate::Volatility;
31-
use crate::{udaf, ExprSchemable, Operator, Signature, WindowFrame, WindowUDF};
31+
use crate::{ExprSchemable, Operator, Signature, WindowFrame, WindowUDF};
3232

3333
use arrow::datatypes::{DataType, Field, FieldRef};
3434
use datafusion_common::cse::{HashNode, NormalizeEq, Normalizeable};
@@ -994,7 +994,7 @@ pub struct AggregateFunctionParams {
994994
/// Optional filter
995995
pub filter: Option<Box<Expr>>,
996996
/// Optional ordering
997-
pub order_by: Option<Vec<Sort>>,
997+
pub order_by: Vec<Sort>,
998998
pub null_treatment: Option<NullTreatment>,
999999
}
10001000

@@ -1005,7 +1005,7 @@ impl AggregateFunction {
10051005
args: Vec<Expr>,
10061006
distinct: bool,
10071007
filter: Option<Box<Expr>>,
1008-
order_by: Option<Vec<Sort>>,
1008+
order_by: Vec<Sort>,
10091009
null_treatment: Option<NullTreatment>,
10101010
) -> Self {
10111011
Self {
@@ -1175,26 +1175,26 @@ impl Exists {
11751175

11761176
/// User Defined Aggregate Function
11771177
///
1178-
/// See [`udaf::AggregateUDF`] for more information.
1178+
/// See [`crate::udaf::AggregateUDF`] for more information.
11791179
#[derive(Clone, PartialEq, Eq, Hash, Debug)]
11801180
pub struct AggregateUDF {
11811181
/// The function
1182-
pub fun: Arc<udaf::AggregateUDF>,
1182+
pub fun: Arc<crate::AggregateUDF>,
11831183
/// List of expressions to feed to the functions as arguments
11841184
pub args: Vec<Expr>,
11851185
/// Optional filter
11861186
pub filter: Option<Box<Expr>>,
11871187
/// Optional ORDER BY applied prior to aggregating
1188-
pub order_by: Option<Vec<Expr>>,
1188+
pub order_by: Vec<Expr>,
11891189
}
11901190

11911191
impl AggregateUDF {
11921192
/// Create a new AggregateUDF expression
11931193
pub fn new(
1194-
fun: Arc<udaf::AggregateUDF>,
1194+
fun: Arc<crate::AggregateUDF>,
11951195
args: Vec<Expr>,
11961196
filter: Option<Box<Expr>>,
1197-
order_by: Option<Vec<Expr>>,
1197+
order_by: Vec<Expr>,
11981198
) -> Self {
11991199
Self {
12001200
fun,
@@ -2303,18 +2303,15 @@ impl NormalizeEq for Expr {
23032303
(None, None) => true,
23042304
_ => false,
23052305
}
2306-
&& match (self_order_by, other_order_by) {
2307-
(Some(self_order_by), Some(other_order_by)) => self_order_by
2308-
.iter()
2309-
.zip(other_order_by.iter())
2310-
.all(|(a, b)| {
2311-
a.asc == b.asc
2312-
&& a.nulls_first == b.nulls_first
2313-
&& a.expr.normalize_eq(&b.expr)
2314-
}),
2315-
(None, None) => true,
2316-
_ => false,
2317-
}
2306+
&& self_order_by
2307+
.iter()
2308+
.zip(other_order_by.iter())
2309+
.all(|(a, b)| {
2310+
a.asc == b.asc
2311+
&& a.nulls_first == b.nulls_first
2312+
&& a.expr.normalize_eq(&b.expr)
2313+
})
2314+
&& self_order_by.len() == other_order_by.len()
23182315
}
23192316
(Expr::WindowFunction(left), Expr::WindowFunction(other)) => {
23202317
let WindowFunction {

‎datafusion/expr/src/expr_fn.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -821,7 +821,7 @@ impl ExprFuncBuilder {
821821

822822
let fun_expr = match fun {
823823
ExprFuncKind::Aggregate(mut udaf) => {
824-
udaf.params.order_by = order_by;
824+
udaf.params.order_by = order_by.unwrap_or_default();
825825
udaf.params.filter = filter.map(Box::new);
826826
udaf.params.distinct = distinct;
827827
udaf.params.null_treatment = null_treatment;

‎datafusion/expr/src/planner.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -294,7 +294,7 @@ pub struct RawAggregateExpr {
294294
pub args: Vec<Expr>,
295295
pub distinct: bool,
296296
pub filter: Option<Box<Expr>>,
297-
pub order_by: Option<Vec<SortExpr>>,
297+
pub order_by: Vec<SortExpr>,
298298
pub null_treatment: Option<NullTreatment>,
299299
}
300300

‎datafusion/expr/src/test/function_stub.rs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ pub fn sum(expr: Expr) -> Expr {
6060
vec![expr],
6161
false,
6262
None,
63-
None,
63+
vec![],
6464
None,
6565
))
6666
}
@@ -73,7 +73,7 @@ pub fn count(expr: Expr) -> Expr {
7373
vec![expr],
7474
false,
7575
None,
76-
None,
76+
vec![],
7777
None,
7878
))
7979
}
@@ -86,7 +86,7 @@ pub fn avg(expr: Expr) -> Expr {
8686
vec![expr],
8787
false,
8888
None,
89-
None,
89+
vec![],
9090
None,
9191
))
9292
}
@@ -282,7 +282,7 @@ pub fn min(expr: Expr) -> Expr {
282282
vec![expr],
283283
false,
284284
None,
285-
None,
285+
vec![],
286286
None,
287287
))
288288
}
@@ -363,7 +363,7 @@ pub fn max(expr: Expr) -> Expr {
363363
vec![expr],
364364
false,
365365
None,
366-
None,
366+
vec![],
367367
None,
368368
))
369369
}

‎datafusion/expr/src/udaf.rs‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ impl AggregateUDF {
158158
args,
159159
false,
160160
None,
161-
None,
161+
vec![],
162162
None,
163163
))
164164
}
@@ -394,7 +394,7 @@ where
394394
/// fn get_doc() -> &'static Documentation {
395395
/// &DOCUMENTATION
396396
/// }
397-
///
397+
///
398398
/// /// Implement the AggregateUDFImpl trait for GeoMeanUdf
399399
/// impl AggregateUDFImpl for GeoMeanUdf {
400400
/// fn as_any(&self) -> &dyn Any { self }
@@ -415,7 +415,7 @@ where
415415
/// ])
416416
/// }
417417
/// fn documentation(&self) -> Option<&Documentation> {
418-
/// Some(get_doc())
418+
/// Some(get_doc())
419419
/// }
420420
/// }
421421
///
@@ -482,7 +482,7 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
482482
schema_name.write_fmt(format_args!(" FILTER (WHERE {filter})"))?;
483483
};
484484

485-
if let Some(order_by) = order_by {
485+
if !order_by.is_empty() {
486486
let clause = match self.is_ordered_set_aggregate() {
487487
true => "WITHIN GROUP",
488488
false => "ORDER BY",
@@ -527,7 +527,7 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
527527
schema_name.write_fmt(format_args!(" FILTER (WHERE {filter})"))?;
528528
};
529529

530-
if let Some(order_by) = order_by {
530+
if !order_by.is_empty() {
531531
schema_name.write_fmt(format_args!(
532532
" ORDER BY [{}]",
533533
schema_name_from_sorts(order_by)?
@@ -616,10 +616,11 @@ pub trait AggregateUDFImpl: Debug + Send + Sync {
616616
if let Some(fe) = filter {
617617
display_name.write_fmt(format_args!(" FILTER (WHERE {fe})"))?;
618618
}
619-
if let Some(ob) = order_by {
619+
if !order_by.is_empty() {
620620
display_name.write_fmt(format_args!(
621621
" ORDER BY [{}]",
622-
ob.iter()
622+
order_by
623+
.iter()
623624
.map(|o| format!("{o}"))
624625
.collect::<Vec<String>>()
625626
.join(", ")

‎datafusion/functions-aggregate/src/approx_percentile_cont.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ pub fn approx_percentile_cont(
6969
args,
7070
false,
7171
None,
72-
Some(vec![order_by]),
72+
vec![order_by],
7373
None,
7474
))
7575
}

‎datafusion/functions-aggregate/src/count.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ pub fn count_distinct(expr: Expr) -> Expr {
7373
vec![expr],
7474
true,
7575
None,
76-
None,
76+
vec![],
7777
None,
7878
))
7979
}

‎datafusion/functions-aggregate/src/first_last.rs‎

Lines changed: 14 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -55,31 +55,23 @@ create_func!(FirstValue, first_value_udaf);
5555
create_func!(LastValue, last_value_udaf);
5656

5757
/// Returns the first value in a group of values.
58-
pub fn first_value(expression: Expr, order_by: Option<Vec<SortExpr>>) -> Expr {
59-
if let Some(order_by) = order_by {
60-
first_value_udaf()
61-
.call(vec![expression])
62-
.order_by(order_by)
63-
.build()
64-
// guaranteed to be `Expr::AggregateFunction`
65-
.unwrap()
66-
} else {
67-
first_value_udaf().call(vec![expression])
68-
}
58+
pub fn first_value(expression: Expr, order_by: Vec<SortExpr>) -> Expr {
59+
first_value_udaf()
60+
.call(vec![expression])
61+
.order_by(order_by)
62+
.build()
63+
// guaranteed to be `Expr::AggregateFunction`
64+
.unwrap()
6965
}
7066

7167
/// Returns the last value in a group of values.
72-
pub fn last_value(expression: Expr, order_by: Option<Vec<SortExpr>>) -> Expr {
73-
if let Some(order_by) = order_by {
74-
last_value_udaf()
75-
.call(vec![expression])
76-
.order_by(order_by)
77-
.build()
78-
// guaranteed to be `Expr::AggregateFunction`
79-
.unwrap()
80-
} else {
81-
last_value_udaf().call(vec![expression])
82-
}
68+
pub fn last_value(expression: Expr, order_by: Vec<SortExpr>) -> Expr {
69+
last_value_udaf()
70+
.call(vec![expression])
71+
.order_by(order_by)
72+
.build()
73+
// guaranteed to be `Expr::AggregateFunction`
74+
.unwrap()
8375
}
8476

8577
#[user_doc(

0 commit comments

Comments
 (0)