-
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
Fix Type Coercion for UDF Arguments #14268
Closed
Closed
Changes from all commits
Commits
Show all changes
37 commits
Select commit
Hold shift + click to select a range
bcc0620
Fix DF 43 regression coerce ascii input to string
shehabgamin a8539c1
datafusion-testing submodule has new commits
shehabgamin 4e4cb02
Merge branch 'main' of github.com:lakehq/datafusion into sail-df-43-r…
shehabgamin eb61d49
implicit cast int to string
shehabgamin a6f62a0
Merge branch 'main' of github.com:lakehq/datafusion into sail-df-43-r…
shehabgamin 5afbfc0
fix can_coerce_to and add tests
shehabgamin 36a23b7
update deprecation message for values exec
shehabgamin d2eadea
Merge branch 'main' of github.com:lakehq/datafusion into sail-df-43-r…
shehabgamin 944e0a3
lint
shehabgamin 2d77206
coerce to string
shehabgamin 1a27626
Adjust type signature
shehabgamin e714ba1
fix comment
shehabgamin 93d75b1
clean up clippy warnings
shehabgamin 5067223
type signature coercible
shehabgamin 4d395e2
bump pyo3
shehabgamin ec8ccd1
udf type coercion
shehabgamin d78877a
moving testing out of functions crate due to circular dependencies
shehabgamin 041e4ac
Merge branch 'main' of github.com:lakehq/datafusion into sail-df-43-r…
shehabgamin 437b83d
find the common string type for TypeSignature::Coercible
shehabgamin 8fd9fb3
update coercible string
shehabgamin 62b97c5
fix error msg and add tests for udfs
shehabgamin 46350c9
update docs to note that args are coercible string
shehabgamin 3f0c870
update expr test
shehabgamin 46cda71
undo
shehabgamin e7d474d
Merge branch 'main' of github.com:lakehq/datafusion into sail-df-43-r…
shehabgamin 1f826cb
remove test since already covered in slt
shehabgamin 5d258b2
add dictionary to base_yupe
shehabgamin 17df1bc
add dictionary to base_type
shehabgamin 97c7db0
fix lint issues
shehabgamin d0f3f9a
Merge branch 'main' of github.com:lakehq/datafusion into sail-df-43-r…
shehabgamin bb773d5
implement TypeSignatureClass::Integer
shehabgamin 08d9b4a
fix slt test
shehabgamin 09ef47c
fix slt
shehabgamin f519d92
Merge branch 'branch-45' of github.com:lakehq/datafusion into sail-df…
shehabgamin 1f806b2
revert native to original and add anynative
shehabgamin 25bcb58
use anynative
shehabgamin 1d94e5c
use user defined rules instead
shehabgamin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -126,9 +126,17 @@ pub enum TypeSignature { | |
Exact(Vec<DataType>), | ||
/// One or more arguments belonging to the [`TypeSignatureClass`], in order. | ||
/// | ||
/// For example, `Coercible(vec![logical_float64()])` accepts | ||
/// arguments like `vec![Int32]` or `vec![Float32]` | ||
/// since i32 and f32 can be cast to f64 | ||
/// `Coercible(vec![TypeSignatureClass::Native(...)])` is designed to cast between the same | ||
/// logical type. | ||
/// | ||
/// For example, `Coercible(vec![TypeSignatureClass::Native(logical_int64())])` accepts | ||
/// arguments like `vec![Int32]` since i32 is the same logical type as i64. | ||
/// | ||
/// Coercible(vec![TypeSignatureClass::Integer(...)])` accepts any | ||
/// integer type (`NativeType::is_integer`) castable to the target integer `NativeType`. | ||
/// | ||
/// For example, `Coercible(vec![TypeSignatureClass::Integer(logical_int64())])` accepts | ||
/// arguments like `vec![Int32]` since i32 can be cast to i64. | ||
/// | ||
/// For functions that take no arguments (e.g. `random()`) see [`TypeSignature::Nullary`]. | ||
Coercible(Vec<TypeSignatureClass>), | ||
|
@@ -213,9 +221,8 @@ pub enum TypeSignatureClass { | |
Interval, | ||
Duration, | ||
Native(LogicalTypeRef), | ||
// TODO: | ||
// Numeric | ||
// Integer | ||
Numeric(LogicalTypeRef), | ||
Integer(LogicalTypeRef), | ||
} | ||
|
||
impl Display for TypeSignatureClass { | ||
|
@@ -376,7 +383,9 @@ impl TypeSignature { | |
TypeSignature::Coercible(types) => types | ||
.iter() | ||
.map(|logical_type| match logical_type { | ||
TypeSignatureClass::Native(l) => get_data_types(l.native()), | ||
TypeSignatureClass::Native(l) | ||
| TypeSignatureClass::Numeric(l) | ||
| TypeSignatureClass::Integer(l) => get_data_types(l.native()), | ||
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.
|
||
TypeSignatureClass::Timestamp => { | ||
vec![ | ||
DataType::Timestamp(TimeUnit::Nanosecond, None), | ||
|
@@ -498,7 +507,7 @@ impl Signature { | |
} | ||
} | ||
|
||
/// A specified number of numeric arguments | ||
/// A specified number of string arguments | ||
pub fn string(arg_count: usize, volatility: Volatility) -> Self { | ||
Self { | ||
type_signature: TypeSignature::String(arg_count), | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@shehabgamin I found that we might not need
LogicalTypeRef
.This is designed to accept all the Integer, so if the given type is integer, we keep it as it is.
If we want specific integer type, then we should use Native instead. Does this makes sense to you?
Numeric is the same