goldmedal commented on code in PR #13372:
URL: https://github.com/apache/datafusion/pull/13372#discussion_r1878309384
##########
datafusion/expr-common/src/signature.rs:
##########
@@ -290,7 +311,24 @@ impl TypeSignature {
.collect(),
TypeSignature::Coercible(types) => types
.iter()
- .map(|logical_type| get_data_types(logical_type.native()))
+ .map(|logical_type| match logical_type {
+ TypeSignatureClass::Native(l) =>
get_data_types(l.native()),
+ TypeSignatureClass::Timestamp => {
+ vec![DataType::Timestamp(TimeUnit::Nanosecond, None)]
Review Comment:
I think we need to provide both `timestamp with time zone` and `timestamp
without time zone` here. It's used to provide the possible type for the
information_schema routine and parameters table.
```rust
vec![DataType::Timestamp(TimeUnit::Nanosecond, None),
DataType::Timestamp(TimeUnit::Nanosecond, Some(TIMEZONE_WILDCARD.into()))]
```
##########
datafusion/expr-common/src/signature.rs:
##########
@@ -138,6 +141,48 @@ pub enum TypeSignature {
NullAry,
}
+impl TypeSignature {
+ #[inline]
+ pub fn is_one_of(&self) -> bool {
+ matches!(self, TypeSignature::OneOf(_))
+ }
+}
+
+#[derive(Debug, Clone, Eq, PartialOrd)]
+pub enum TypeSignatureClass {
+ Timestamp,
Review Comment:
I also feel like we should have another variant for `timestamp with time
zone`. IMO, they are different types.
- `Timestamp(timeunit, None)` for `timestamp without time zone`.
- `Timestamp(timeunit, Some(TIMEZONE_WILDCARD)` for `timestamp with time
zone`.
They can't interact with each other before applying some casting or coercion.
For example,
https://github.com/apache/datafusion/blob/91670e2d152d5819b0f102268f5eeb299f0d6823/datafusion/functions/src/datetime/date_bin.rs#L59-L68
The `date_bin` function accepts two `Timestamp` arguments. However, if we
try to simplify here, we may write something like
```
TypeSignature::Coercible(vec![
TypeSignatureClass::Interval,
TypeSignatureClass::Timstamp,
TypeSignatureClass::Timstamp,
]),
```
It means we can accept the SQL like `date_bin(INTERVAL 1 HOUR,
timestamp_without_timezone_col, timestamp_with_timezone_col)` but I guess it's
not correct. (no match the original signature)
If we have a class for `timestamp with time zone`, we can write
```rust
TypeSianture::one_of([
TypeSignature::Coercible(vec![
TypeSignatureClass::Interval,
TypeSignatureClass::Timstamp,
TypeSignatureClass::Timstamp,
]),
TypeSignature::Coercible(vec![
TypeSignatureClass::Interval,
TypeSignatureClass::Timstamp_with_time_zone,
TypeSignatureClass::Timstamp_with_time_zone,
]),
])
```
It's more close to the original signature.
--
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
