Introduce Signature::Coercible#12275
Conversation
github-actions
bot
added
logical-expr
Signed-off-by: jayzhan211 <[email protected]>
jayzhan211
force-pushed
the
signature-float
branch
from
e2c0385 to
9af788b
Compare
Signed-off-by: jayzhan211 <[email protected]>
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks good to me -- thank you @jayzhan211
| /// 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 | ||
| Numeric(usize), | ||
| Float(usize), |
There was a problem hiding this comment.
One minor comment is perhaps we should call this Float64 to be explicit that the argument will be 64 bit (rather than maybe f32 or maybe f64)
There was a problem hiding this comment.
I'm concern that we will ends up with specific signature Float32, Float64, Float16... Another idea I'm working on is introducing TypeSignature::Coercible(Vec<DataType>), similar to Exact but try to coerce to target type.
So we could have TypeSignature::Coercible(vec![Float64]) in this case, and this is a more general signature
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
| | TypeSignature::Coercible(_) => { | ||
| // User-defined signature is validated in `coerce_types` | ||
| // Numreic signature is validated in `get_valid_types` | ||
| // Numreic and Coercible signature is validated in `get_valid_types` |
There was a problem hiding this comment.
| // Numreic and Coercible signature is validated in `get_valid_types` | |
| // Numeric and Coercible signature is validated in `get_valid_types` |
| Uniform(usize, Vec<DataType>), | ||
| /// Exact number of arguments of an exact type | ||
| Exact(Vec<DataType>), | ||
| Coercible(Vec<DataType>), |
There was a problem hiding this comment.
perhaps we could add some comments here explaining what Coercible meant? Maybe it means that the input arguments can be coerced to one of the target types (in order)?
There was a problem hiding this comment.
Reading this again, I am not sure how this is different than Exact(Vec<DataType>) as I thought the coercion logic would try to coerce the arguments to the types in Exact 🤔
Is the difference is that the input isn't actually converted to the Coercable types by DataFusion, but instead the function's implementation handles coercion internally?
|
This idea looks much nicer and general @jayzhan211 |
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
2 participants
Which issue does this PR close?
Part of #10507
Rationale for this change
We can see that in DuckDB, they return float type for
avgeven the input type is not float.We return Integer in Datafusion. I think return Float for integer input has little downside but could make the type handling a lot simpler, since we have less types to care about.
What changes are included in this PR?
Add Signature::float that we accept type that is coercible to f64.
Are these changes tested?
Are there any user-facing changes?