alamb commented on code in PR #9411:
URL: https://github.com/apache/arrow-datafusion/pull/9411#discussion_r1510297526
##########
datafusion/physical-expr/src/aggregate/first_last.rs:
##########
@@ -44,6 +44,7 @@ pub struct FirstValue {
expr: Arc<dyn PhysicalExpr>,
ordering_req: LexOrdering,
requirement_satisfied: bool,
+ ignore_null: bool,
Review Comment:
a very minor nit is that some parts of this PR use `ignore_null` and some
parts use `ignore_nulls` (with an `s`). It might be nice to make them uniform
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -246,6 +246,7 @@ fn create_physical_name(e: &Expr, is_first_expr: bool) ->
Result<String> {
args,
filter,
order_by,
+ ..
Review Comment:
As as style thing I have found it helpful to not use a `..` match all, and
instead explicitly ignore each field like
```suggestion
null_treatment: _,
```
With this treatment, if/when someone adds a new field to `AggregateFunction`
in the future, the compiler will tell them all the places that may need to be
changed.
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -672,7 +672,7 @@ pub fn to_substrait_agg_measure(
),
) -> Result<Measure> {
match expr {
- Expr::AggregateFunction(expr::AggregateFunction { func_def, args,
distinct, filter, order_by }) => {
+ Expr::AggregateFunction(expr::AggregateFunction { func_def, args,
distinct, filter, order_by, .. }) => {
Review Comment:
same comment here about matching all vs explicit field matches
--
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]
