-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use LogicalType for TypeSignature Numeric
and String
, Coercible
#13240
Changes from 3 commits
30be8a0
4b6d433
5cb2aa9
c9ac3c5
ec65ca1
1aba4cb
acd2ceb
3855c6f
804242c
793b0da
2cfa273
6e97cb1
99f2cd3
f96e5d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -348,6 +348,12 @@ impl LogicalType for NativeType { | |||||
// mapping solutions to provide backwards compatibility while transitioning from | ||||||
// the purely physical system to a logical / physical system. | ||||||
|
||||||
impl From<&DataType> for NativeType { | ||||||
fn from(value: &DataType) -> Self { | ||||||
value.clone().into() | ||||||
} | ||||||
} | ||||||
|
||||||
impl From<DataType> for NativeType { | ||||||
fn from(value: DataType) -> Self { | ||||||
use NativeType::*; | ||||||
|
@@ -392,8 +398,38 @@ impl From<DataType> for NativeType { | |||||
} | ||||||
} | ||||||
|
||||||
impl From<&DataType> for NativeType { | ||||||
fn from(value: &DataType) -> Self { | ||||||
value.clone().into() | ||||||
impl NativeType { | ||||||
#[inline] | ||||||
pub fn is_numeric(&self) -> bool { | ||||||
use NativeType::*; | ||||||
matches!( | ||||||
self, | ||||||
UInt8 | ||||||
| UInt16 | ||||||
| UInt32 | ||||||
| UInt64 | ||||||
| Int8 | ||||||
| Int16 | ||||||
| Int32 | ||||||
| Int64 | ||||||
| Float16 | ||||||
| Float32 | ||||||
| Float64 | ||||||
| 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! | ||||||
pub fn can_cast_to(&self, target_type: &Self) -> bool { | ||||||
// In Postgres, most functions coerce numeric strings to numeric inputs, | ||||||
// but they do not accept numeric inputs as strings. | ||||||
if self.is_numeric() && target_type == &NativeType::String { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This talks about "can cast" and "can coerce" without making clear distinction between them. |
||||||
return false; | ||||||
} | ||||||
|
||||||
true | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Default for "can cast" and "can coerce" should be false. Let'e have
Suggested change
here and let's define what can be converted in rules above. This will make the code simpler to reason about |
||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,8 +33,8 @@ use datafusion_expr::aggregate_doc_sections::DOC_SECTION_GENERAL; | |
use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs}; | ||
use datafusion_expr::utils::{format_state_name, AggregateOrderSensitivity}; | ||
use datafusion_expr::{ | ||
Accumulator, AggregateUDFImpl, ArrayFunctionSignature, Documentation, Expr, | ||
ExprFunctionExt, Signature, SortExpr, TypeSignature, Volatility, | ||
Accumulator, AggregateUDFImpl, Documentation, Expr, ExprFunctionExt, Signature, | ||
SortExpr, Volatility, | ||
}; | ||
use datafusion_functions_aggregate_common::utils::get_sort_options; | ||
use datafusion_physical_expr_common::sort_expr::{LexOrdering, LexOrderingRef}; | ||
|
@@ -79,15 +79,7 @@ impl Default for FirstValue { | |
impl FirstValue { | ||
pub fn new() -> Self { | ||
Self { | ||
signature: Signature::one_of( | ||
vec![ | ||
// TODO: we can introduce more strict signature that only numeric of array types are allowed | ||
TypeSignature::ArraySignature(ArrayFunctionSignature::Array), | ||
TypeSignature::Numeric(1), | ||
TypeSignature::Uniform(1, vec![DataType::Utf8]), | ||
], | ||
Volatility::Immutable, | ||
), | ||
signature: Signature::any(1, Volatility::Immutable), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally first/last value get the first/last item in the column, so it should be the type of the column. I forgot why we don't use |
||
requirement_satisfied: false, | ||
} | ||
} | ||
|
@@ -406,15 +398,7 @@ impl Default for LastValue { | |
impl LastValue { | ||
pub fn new() -> Self { | ||
Self { | ||
signature: Signature::one_of( | ||
vec![ | ||
// TODO: we can introduce more strict signature that only numeric of array types are allowed | ||
TypeSignature::ArraySignature(ArrayFunctionSignature::Array), | ||
TypeSignature::Numeric(1), | ||
TypeSignature::Uniform(1, vec![DataType::Utf8]), | ||
], | ||
Volatility::Immutable, | ||
), | ||
signature: Signature::any(1, Volatility::Immutable), | ||
requirement_satisfied: false, | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -186,7 +186,7 @@ impl ScalarUDFImpl for ConcatFunc { | |
} | ||
}; | ||
} | ||
_ => unreachable!(), | ||
_ => unreachable!("concat"), | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can make a distinction between
Debug
andDisplay
. Currently, we print something likeI imagine we can print
Int32
toInt32
, printJSON
toJSON
... 🤔However, I think it may not be the point of this PR. We can do it in another PR.