Skip to content

Commit

Permalink
deprecate coercible
Browse files Browse the repository at this point in the history
Signed-off-by: jayzhan211 <[email protected]>
  • Loading branch information
jayzhan211 committed Nov 4, 2024
1 parent c9ac3c5 commit ec65ca1
Show file tree
Hide file tree
Showing 5 changed files with 14 additions and 80 deletions.
22 changes: 0 additions & 22 deletions datafusion/common/src/types/native.rs
Original file line number Diff line number Diff line change
Expand Up @@ -418,26 +418,4 @@ impl NativeType {
| Decimal(_, _)
)
}

/// This function is the NativeType version of `can_cast_types`.
/// It handles general coercion rules that are widely applicable.
/// Avoid adding specific coercion cases here.
/// Aim to keep this logic as SIMPLE as possible!
///
/// Ensure there is a corresponding test for this function.
pub fn can_coerce_to(&self, target_type: &Self) -> bool {
if self.eq(target_type) {
return true;
}

if self.is_numeric() && target_type.is_numeric() {
return true;
}

if self.eq(&NativeType::Null) && target_type == &NativeType::String {
return true;
}

false
}
}
3 changes: 2 additions & 1 deletion datafusion/expr-common/src/signature.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ pub enum TypeSignature {
/// Specifies Signatures for array functions
ArraySignature(ArrayFunctionSignature),
/// Fixed number of arguments of numeric types.
/// See <https://docs.rs/arrow/latest/arrow/datatypes/enum.DataType.html#method.is_numeric> to know which type is considered numeric
/// See [`NativeType::is_numeric`] to know which type is considered numeric
Numeric(usize),
/// Fixed number of arguments of all the same string types.
/// The precedence of type from high to low is Utf8View, LargeUtf8 and Utf8.
Expand Down Expand Up @@ -326,6 +326,7 @@ impl Signature {
}
}
/// Target coerce types in order
#[deprecated(since = "42.0.0", note = "Use String, Numeric")]
pub fn coercible(target_types: Vec<LogicalTypeRef>, volatility: Volatility) -> Self {
Self {
type_signature: TypeSignature::Coercible(target_types),
Expand Down
55 changes: 9 additions & 46 deletions datafusion/expr/src/type_coercion/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use arrow::{
};
use datafusion_common::{
exec_err, internal_datafusion_err, internal_err, plan_err,
types::{LogicalType, NativeType},
types::NativeType,
utils::{coerced_fixed_size_list_to_list, list_ndims},
Result,
};
Expand Down Expand Up @@ -402,10 +402,6 @@ fn get_valid_types(
.map(|valid_type| current_types.iter().map(|_| valid_type.clone()).collect())
.collect(),
TypeSignature::String(number) => {
// TODO: we can switch to coercible after all the string functions support utf8view since it is choosen as the default string type.
//
// let data_types = get_valid_types(&TypeSignature::Coercible(vec![logical_string(); *number]), current_types)?.swap_remove(0);

if *number < 1 {
return plan_err!(
"The signature expected at least one argument but received {}",
Expand All @@ -423,12 +419,11 @@ fn get_valid_types(
let mut new_types = Vec::with_capacity(current_types.len());
for data_type in current_types.iter() {
let logical_data_type: NativeType = data_type.into();
if logical_data_type.can_coerce_to(&NativeType::String) {
if data_type.is_null() {
new_types.push(DataType::Utf8);
} else {
new_types.push(data_type.to_owned());
}
if logical_data_type == NativeType::String {
new_types.push(data_type.to_owned());
} else if logical_data_type == NativeType::Null {
// TODO: Switch to Utf8View if all the string functions supports Utf8View
new_types.push(DataType::Utf8);
} else {
return plan_err!(
"The signature expected NativeType::String but received {data_type}"
Expand Down Expand Up @@ -526,48 +521,16 @@ fn get_valid_types(

let logical_data_type: NativeType = valid_type.clone().into();
// Fallback to default type if we don't know which type to coerced to
// f64 is choosen since most of the math function utilize Signature::numeric,
// f64 is choosen since most of the math function utilize Signature::numeric,
// and their default type is double precision
if matches!(logical_data_type, NativeType::String | NativeType::Null) {
valid_type = DataType::Float64;
}

vec![vec![valid_type; *number]]
}
TypeSignature::Coercible(target_types) => {
if target_types.is_empty() {
return plan_err!(
"The signature expected at least one argument but received {}",
current_types.len()
);
}
if target_types.len() != current_types.len() {
return plan_err!(
"The signature expected {} arguments but received {}",
target_types.len(),
current_types.len()
);
}

let mut new_types = Vec::with_capacity(current_types.len());
for (data_type, target_type) in current_types.iter().zip(target_types.iter())
{
let logical_data_type: NativeType = data_type.into();
if logical_data_type == *target_type.native() {
new_types.push(data_type.to_owned());
} else if logical_data_type.can_coerce_to(target_type.native()) {
let casted_type = target_type.default_cast_for(data_type)?;
new_types.push(casted_type);
} else {
return plan_err!(
"The signature expected {:?} but received {:?}",
target_type.native(),
logical_data_type
);
}
}

vec![new_types]
TypeSignature::Coercible(_target_types) => {
return plan_err!("Deprecated, use String, Numeric directly");
}
TypeSignature::Uniform(number, valid_types) => valid_types
.iter()
Expand Down
6 changes: 1 addition & 5 deletions datafusion/functions-aggregate/src/stddev.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ use std::sync::{Arc, OnceLock};
use arrow::array::Float64Array;
use arrow::{array::ArrayRef, datatypes::DataType, datatypes::Field};

use datafusion_common::types::logical_float64;
use datafusion_common::{internal_err, not_impl_err, Result};
use datafusion_common::{plan_err, ScalarValue};
use datafusion_expr::aggregate_doc_sections::DOC_SECTION_STATISTICAL;
Expand Down Expand Up @@ -72,10 +71,7 @@ impl Stddev {
/// Create a new STDDEV aggregate function
pub fn new() -> Self {
Self {
signature: Signature::coercible(
vec![logical_float64()],
Volatility::Immutable,
),
signature: Signature::numeric(1, Volatility::Immutable),
alias: vec!["stddev_samp".to_string()],
}
}
Expand Down
8 changes: 2 additions & 6 deletions datafusion/functions-aggregate/src/variance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ use std::sync::OnceLock;
use std::{fmt::Debug, sync::Arc};

use datafusion_common::{
downcast_value, not_impl_err, plan_err, types::logical_float64, DataFusionError,
Result, ScalarValue,
downcast_value, not_impl_err, plan_err, DataFusionError, Result, ScalarValue,
};
use datafusion_expr::aggregate_doc_sections::DOC_SECTION_GENERAL;
use datafusion_expr::{
Expand Down Expand Up @@ -83,10 +82,7 @@ impl VarianceSample {
pub fn new() -> Self {
Self {
aliases: vec![String::from("var_sample"), String::from("var_samp")],
signature: Signature::coercible(
vec![logical_float64()],
Volatility::Immutable,
),
signature: Signature::numeric(1, Volatility::Immutable),
}
}
}
Expand Down

0 comments on commit ec65ca1

Please sign in to comment.