Jefffrey commented on code in PR #18647:
URL: https://github.com/apache/datafusion/pull/18647#discussion_r2518159521
##########
datafusion/functions-aggregate/src/approx_median.rs:
##########
@@ -57,20 +59,11 @@ make_udaf_expr_and_func!(
```"#,
standard_argument(name = "expression",)
)]
-#[derive(PartialEq, Eq, Hash)]
+#[derive(Debug, PartialEq, Eq, Hash)]
pub struct ApproxMedian {
signature: Signature,
}
-impl Debug for ApproxMedian {
Review Comment:
I don't see much value in these manual implementations; I guess it adds the
`name` field but I feel its better to just `derive` for simplicity. Happy to
revert if desired.
##########
datafusion/functions-aggregate-common/src/tdigest.rs:
##########
@@ -61,41 +60,6 @@ macro_rules! cast_scalar_u64 {
};
}
-/// This trait is implemented for each type a [`TDigest`] can operate on,
-/// allowing it to support both numerical rust types (obtained from
-/// `PrimitiveArray` instances), and [`ScalarValue`] instances.
-pub trait TryIntoF64 {
- /// A fallible conversion of a possibly null `self` into a [`f64`].
- ///
- /// If `self` is null, this method must return `Ok(None)`.
- ///
- /// If `self` cannot be coerced to the desired type, this method must
return
- /// an `Err` variant.
- fn try_as_f64(&self) -> Result<Option<f64>>;
-}
-
-/// Generate an infallible conversion from `type` to an [`f64`].
-macro_rules! impl_try_ordered_f64 {
- ($type:ty) => {
- impl TryIntoF64 for $type {
- fn try_as_f64(&self) -> Result<Option<f64>> {
- Ok(Some(*self as f64))
- }
- }
- };
-}
Review Comment:
This seemed a bit pointless API (maybe it was more useful in the past) so I
just inlined it
##########
datafusion/functions-aggregate/src/approx_median.rs:
##########
@@ -81,33 +74,53 @@ impl ApproxMedian {
/// Create a new APPROX_MEDIAN aggregate function
pub fn new() -> Self {
Self {
- signature: Signature::uniform(1, NUMERICS.to_vec(),
Volatility::Immutable),
+ signature: Signature::one_of(
+ vec![
+ TypeSignature::Coercible(vec![Coercion::new_exact(
+ TypeSignatureClass::Integer,
+ )]),
+ TypeSignature::Coercible(vec![Coercion::new_implicit(
+ TypeSignatureClass::Float,
+ vec![TypeSignatureClass::Decimal],
+ NativeType::Float64,
+ )]),
+ ],
+ Volatility::Immutable,
Review Comment:
Numerics list excludes decimals, so we only support decimals via casting to
float (same behaviour as before)
##########
datafusion/functions-aggregate/src/approx_percentile_cont.rs:
##########
@@ -376,83 +366,51 @@ impl ApproxPercentileAccumulator {
match values.data_type() {
DataType::Float64 => {
let array = downcast_value!(values, Float64Array);
- Ok(array
- .values()
- .iter()
- .filter_map(|v| v.try_as_f64().transpose())
- .collect::<Result<Vec<_>>>()?)
+ Ok(array.values().iter().copied().collect::<Vec<_>>())
}
DataType::Float32 => {
let array = downcast_value!(values, Float32Array);
+ Ok(array.values().iter().map(|v| *v as
f64).collect::<Vec<_>>())
+ }
+ DataType::Float16 => {
+ let array = downcast_value!(values, Float16Array);
Ok(array
.values()
.iter()
- .filter_map(|v| v.try_as_f64().transpose())
- .collect::<Result<Vec<_>>>()?)
+ .map(|v| v.to_f64())
+ .collect::<Vec<_>>())
Review Comment:
See the inlining here
--
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]
