goldmedal commented on code in PR #13240:
URL: https://github.com/apache/datafusion/pull/13240#discussion_r1829414050
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -489,31 +548,55 @@ 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,
Review Comment:
```suggestion
// f64 is chosen since most of the math functions utilize
Signature::numeric,
```
typo
##########
datafusion/common/src/types/logical.rs:
##########
@@ -98,6 +98,12 @@ impl fmt::Debug for dyn LogicalType {
}
}
+impl std::fmt::Display for dyn LogicalType {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ write!(f, "{self:?}")
Review Comment:
Maybe we can make a distinction between `Debug` and `Display`. Currently, we
print something like
```rust
let int: Box<dyn LogicalType> = Box::new(Int32);
println!(format!("{}", int));
------- output ------
LogicalType(Native(Int32), Int32))
```
I imagine we can print `Int32` to`Int32`, print `JSON` to `JSON` ... 🤔
However, I think it may not be the point of this PR. We can do it in another
PR.
##########
datafusion/expr-common/src/signature.rs:
##########
@@ -123,8 +124,19 @@ 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
+ ///
+ /// [`NativeType::is_numeric`]: datafusion_common
Numeric(usize),
+ /// Fixed number of arguments of numeric types.
+ /// See [`NativeType::is_numeric`] to know which type is considered numeric
+ /// This signature accepts numeric string
+ /// Example of functions In Postgres that support numeric string
+ /// 1. Mathematical Functions, like `abs`
Review Comment:
I agree with @findepi.
It makes sense to me. `NumericAndNumericString` allows `abs('-1')` but also
allows `abs('-1'::varchar)`, which isn't allowed by Postgres.
A similar behavior in DuckDB is with `to_timestamp`.
```
D select to_timestamp('-1');
┌──────────────────────────┐
│ to_timestamp('-1') │
│ timestamp with time zone │
├──────────────────────────┤
│ 1970-01-01 07:59:59+08 │
└──────────────────────────┘
D select to_timestamp('-1'::varchar);
Binder Error: No function matches the given name and argument types
'to_timestamp(VARCHAR)'. You might need to add explicit type casts.
Candidate functions:
to_timestamp(DOUBLE) -> TIMESTAMP WITH TIME ZONE
LINE 1: select to_timestamp('-1'::varchar);
```
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
