Jefffrey commented on code in PR #19727:
URL: https://github.com/apache/datafusion/pull/19727#discussion_r2678425557
##########
datafusion/functions-aggregate/src/approx_percentile_cont_with_weight.rs:
##########
@@ -111,20 +111,12 @@ An alternative syntax is also supported:
description = "Number of centroids to use in the t-digest algorithm.
_Default is 100_. A higher number results in more accurate approximation but
requires more memory."
)
)]
-#[derive(PartialEq, Eq, Hash)]
+#[derive(PartialEq, Eq, Hash, Debug)]
pub struct ApproxPercentileContWithWeight {
signature: Signature,
approx_percentile_cont: ApproxPercentileCont,
}
-impl Debug for ApproxPercentileContWithWeight {
Review Comment:
The only advantage I see to these manual impls is sometimes they add the
name field; I personally don't see those as useful enough to need a separate
impl so opting for derive to remove code where possible.
##########
datafusion/functions-aggregate/src/covariance.rs:
##########
@@ -112,11 +100,7 @@ impl AggregateUDFImpl for CovarianceSample {
&self.signature
}
- fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
- if !arg_types[0].is_numeric() {
Review Comment:
We should be more confident in our signature code to guard this for us, to
promote consistency across our UDFs
##########
datafusion/functions-aggregate/src/covariance.rs:
##########
@@ -94,7 +79,10 @@ impl CovarianceSample {
pub fn new() -> Self {
Self {
aliases: vec![String::from("covar")],
- signature: Signature::uniform(2, NUMERICS.to_vec(),
Volatility::Immutable),
+ signature: Signature::exact(
+ vec![DataType::Float64, DataType::Float64],
+ Volatility::Immutable,
+ ),
Review Comment:
This way type coercion handles casting for us; we can now remove the code
that does casting internally in the accumulators for us
##########
datafusion/functions-aggregate/src/covariance.rs:
##########
@@ -386,10 +330,10 @@ impl Accumulator for CovarianceAccumulator {
}
fn merge_batch(&mut self, states: &[ArrayRef]) -> Result<()> {
- let counts = downcast_value!(states[0], UInt64Array);
- let means1 = downcast_value!(states[1], Float64Array);
- let means2 = downcast_value!(states[2], Float64Array);
- let cs = downcast_value!(states[3], Float64Array);
+ let counts = as_uint64_array(&states[0])?;
Review Comment:
I started using `as_float64_array` above so decided to make the whole file
consistent in which way it handles downcasting
##########
datafusion/functions-aggregate/src/covariance.rs:
##########
@@ -304,30 +278,15 @@ impl Accumulator for CovarianceAccumulator {
}
fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
- let values1 = &cast(&values[0], &DataType::Float64)?;
- let values2 = &cast(&values[1], &DataType::Float64)?;
-
- let mut arr1 = downcast_value!(values1, Float64Array).iter().flatten();
- let mut arr2 = downcast_value!(values2, Float64Array).iter().flatten();
+ let values1 = as_float64_array(&values[0])?;
+ let values2 = as_float64_array(&values[1])?;
- for i in 0..values1.len() {
- let value1 = if values1.is_valid(i) {
- arr1.next()
- } else {
- None
- };
- let value2 = if values2.is_valid(i) {
- arr2.next()
- } else {
- None
+ for (value1, value2) in values1.iter().zip(values2) {
Review Comment:
Little driveby refactor to make iteration cleaner
--
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]
