martin-g commented on code in PR #18446:
URL: https://github.com/apache/datafusion/pull/18446#discussion_r2499083174
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
Review Comment:
I believe these could make use of the new helper function too:
```rust
let exprs_list: Vec<_> = group_by.expr.iter().map(|(e, _)|
Arc::clone(e)).collect();
let null_exprs_list: Vec<_> = group_by.null_expr.iter().map(|(e, _)|
Arc::clone(e)).collect();
let exprs: Vec<ArrayRef> = evaluate_expressions_to_arrays(&exprs_list,
batch)?;
let null_exprs: Vec<ArrayRef> =
evaluate_expressions_to_arrays(&null_exprs_list, batch)?;
```
##########
datafusion/physical-plan/src/aggregates/no_grouping.rs:
##########
@@ -219,13 +219,8 @@ fn aggregate_batch(
None => Cow::Borrowed(&batch),
};
- let n_rows = batch.num_rows();
-
// 1.3
- let values = expr
- .iter()
- .map(|e| e.evaluate(&batch).and_then(|v| v.into_array(n_rows)))
- .collect::<Result<Vec<_>>>()?;
+ let values = evaluate_expressions_to_arrays(expr, &batch)?;
Review Comment:
```suggestion
let values = evaluate_expressions_to_arrays(expr,
batch.as_ref())?;
```
nit: to make it more obvious that `batch` is a Cow
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
