Simplify type signatures using `TypeSignatureClass` for mixed type function signature #13372

jayzhan211 · 2024-11-12T07:03:49Z

Which issue does this PR close?

Part of #13301

Closes #.

Rationale for this change

Some functions require mixed type signature such as (string, integer) or (string, timestamp).

Time related type is added in this PR as the first step.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Signed-off-by: jayzhan211 <[email protected]>

findepi · 2024-11-16T18:16:39Z

datafusion/expr-common/src/signature.rs

+
+#[derive(Debug, Clone, Eq, PartialOrd)]
+pub enum TypeSignatureClass {
+    Timestamp,


We may need to treat timestamp and timestamp with zone separately 🤔

Unless there is function that treat timestamp differently based on timezone, otherwise it is simpler to treat them equivalently.

I think some functions currently define that they take a UTC timestamp or ANY timestamp via https://docs.rs/datafusion/latest/datafusion/logical_expr/constant.TIMEZONE_WILDCARD.html

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,

datafusion/datafusion/functions/src/datetime/date_bin.rs

Lines 59 to 68 in 91670e2

Exact(vec![

DataType::Interval(MonthDayNano),

Timestamp(array_type, None),

Timestamp(Nanosecond, None),

]),

Exact(vec![

DataType::Interval(MonthDayNano),

Timestamp(array_type, Some(TIMEZONE_WILDCARD.into())),

Timestamp(Nanosecond, Some(TIMEZONE_WILDCARD.into())),

]),

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

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.

How about managing timestamps within the invoke function? Handling specific cases like Timestamp_with_time_zone or TIMEZONE_WILDCARD adds unnecessary complexity to the function's signature without providing much benefit. Instead, why not define a high-level Timestamp in the signature and handle the finer details elsewhere?

Maybe we can refer to the behavior of Postgres or DuckDB. I listed the signature of the timestamp function age from their system catalogs for reference.

Postgres

test=# SELECT proname, proargtypes FROM pg_proc WHERE proname = 'age'; proname | proargtypes ---------+------------- age | 28 age | 1114 age | 1184 age | 1114 1114 age | 1184 1184 (5 rows) test=# SELECT oid, typname FROM pg_type WHERE oid IN (28, 1184, 1114); oid | typname ------+------------- 28 | xid 1114 | timestamp 1184 | timestamptz (3 rows)

DuckDB

D SELECT function_name, parameter_types FROM duckdb_functions() WHERE function_name = 'age'; ┌───────────────┬──────────────────────────────────────────────────────┐ │ function_name │ parameter_types │ │ varchar │ varchar[] │ ├───────────────┼──────────────────────────────────────────────────────┤ │ age │ [TIMESTAMP] │ │ age │ [TIMESTAMP, TIMESTAMP] │ │ age │ [TIMESTAMP WITH TIME ZONE, TIMESTAMP WITH TIME ZONE] │ │ age │ [TIMESTAMP WITH TIME ZONE] │ └───────────────┴──────────────────────────────────────────────────────┘

If you try to input an unsupported type value, you may encounter an error like the following:

D SELECT age('2001-01-01 18:00:00'); Binder Error: Could not choose a best candidate function for the function call "age(STRING_LITERAL)". In order to select one, please add explicit type casts. Candidate functions: age(TIMESTAMP WITH TIME ZONE) -> INTERVAL age(TIMESTAMP) -> INTERVAL LINE 1: SELECT age('2001-01-01 18:00:00');

Both systems treat TIMESTAMP and TIMESTAMP WITH TIME ZONE as distinct types in the high level.

The advantage of separating these types is that it allows for stricter type-checking when matching a function's signature. This reduces the likelihood of developers failing to correctly handle type checks when implementing timestamp functions.

Honestly, I'm not sure how complex it would become if we separated them 🤔 . If it requires significant effort, I'm fine with keeping the current design.

Both systems treat TIMESTAMP and TIMESTAMP WITH TIME ZONE as distinct types in the high level.

that's a good point, they are quite different: only the latter denotes point in time. the former denotes "wall date/time" with no zone information, so does not denote any particular point in time.

their similarity is deceptive and source of many bugs

This reduces the likelihood of developers failing to correctly handle type checks when implementing timestamp functions.

the timestamp and timestamp tz values are different arrow types, so the function implementor needs to handle them separately anyway

The point is, some functions will be applicable to one of these types but not the other.
for example, a (hypothetical) to_unix_timestamp(timestamp_tz) -> Int64 function should operate on point in time value, so it should accept the timestamp_tz.
Note that in SQL, timestamp is coercible to timestamp_tz, so such function is still going to be callable with timestamp value, but that's not something function implementor should be concerned about.

My solution so far is that we use TypeSignatureClass::Timestamp if we don't care about it has timezone or not. Fallback to TypeSignatureClass::Native() if we need to tell the difference

My solution so far is that we use TypeSignature::Timestamp if we don't care about it has timezone or not. Fallback to TypeSignature::Native() if we need to tell the difference

Agreed. Handling Timestamps using TypeSignatureClass::Native is a good idea. I still think we should treat them as different types.

findepi · 2024-11-16T18:16:46Z

datafusion/expr-common/src/signature.rs

@@ -112,8 +114,9 @@ pub enum TypeSignature {
    /// For example, `Coercible(vec![logical_float64()])` accepts
    /// arguments like `vec![DataType::Int32]` or `vec![DataType::Float32]`
    /// since i32 and f32 can be casted to f64
-    Coercible(Vec<LogicalTypeRef>),
-    /// Fixed number of arguments of arbitrary types, number should be larger than 0
+    Coercible(Vec<TypeSignatureClass>),


datafusion/common/src/types/native.rs

findepi · 2024-11-16T18:18:04Z

datafusion/sqllogictest/test_files/expr.slt

 ----
 12.12345678

 query R
-SELECT date_part('millisecond', '2020-09-08T12:00:12.12345678+00:00')
+SELECT date_part('millisecond', timestamp '2020-09-08T12:00:12.12345678+00:00')


Why change existing test queries?

because the valid type should be timestamp but not string.

This is the result in Postgres

postgres=# SELECT date_part('millisecond', timestamp '2020-09-08T12:00:12.12345678+00:00'); date_part ----------- 12123.457 (1 row) postgres=# SELECT date_part('millisecond', '2020-09-08T12:00:12.12345678+00:00'); ERROR: function date_part(unknown, unknown) is not unique LINE 1: SELECT date_part('millisecond', '2020-09-08T12:00:12.1234567... ^ HINT: Could not choose a best candidate function. You might need to add explicit type casts.

findepi · 2024-11-16T18:20:12Z

datafusion/expr-common/src/signature.rs

+
+impl Display for TypeSignatureClass {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "TypeSignatureClass::{self:?}")


Display looks more verbose than Debug. Typically it's the other way around.

the produced err msg looks a bit longish, but i don't know how to make it more readabile. thoughts?

Internal error: Expect TypeSignatureClass::Native\(LogicalType\(Native\(Int64\), Int64\)\) but received Float64

I agree it would be great to have a special type signature class display

Maybe something like (any int) or Integer or (any timestamp) for Timestamp 🤔

alamb · 2024-11-20T22:24:22Z

@jayzhan211 is this one ready for review by a committer ? I am having trouble keeping track of everything that is going on and am trying to prioritize reviews where it would be most helpful.

If you have any suggestions, please let me know

jayzhan211 · 2024-11-21T00:03:25Z

@jayzhan211 is this one ready for review by a committer ? I am having trouble keeping track of everything that is going on and am trying to prioritize reviews where it would be most helpful.

If you have any suggestions, please let me know

Yes, this is

trying to prioritize reviews where it would be most helpful.

I think we should. I will ping directly if I think it should be prioritized

- Added logic to prevent unnecessary casting of string types in `native.rs`. - Introduced `Comparable` variant in `TypeSignature` to define coercion rules for comparisons. - Updated imports in `functions.rs` and `signature.rs` for better organization. - Modified `date_part.rs` to improve handling of timestamp extraction and fixed query tests in `expr.slt`. - Added `datafusion-macros` dependency in `Cargo.toml` and `Cargo.lock`. These changes improve type handling and ensure more accurate function behavior in SQL expressions.

jayzhan211 · 2024-12-09T13:37:10Z

datafusion/functions/src/datetime/date_part.rs

-                    Exact(vec![
-                        Utf8,
-                        Timestamp(Nanosecond, Some(TIMEZONE_WILDCARD.into())),
+                    TypeSignature::Coercible(vec![


Simplified signature is the main goal of this PR

alamb

Thanks @jayzhan211 -- this is looking quite good. I am worried about the regression of implicit casting from string to timestamps as well as the fact that I think this may not work for timestamps without timezones (seems like there is a gap in testing for datepart -- I will make a PR to add some more tests)

alamb · 2024-12-09T15:50:43Z

datafusion/expr-common/src/signature.rs

+
+#[derive(Debug, Clone, Eq, PartialOrd)]
+pub enum TypeSignatureClass {
+    Timestamp,


I think some functions currently define that they take a UTC timestamp or ANY timestamp via https://docs.rs/datafusion/latest/datafusion/logical_expr/constant.TIMEZONE_WILDCARD.html

datafusion/expr-common/src/signature.rs

alamb · 2024-12-09T15:56:19Z

datafusion/expr-common/src/signature.rs

+
+impl Display for TypeSignatureClass {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "TypeSignatureClass::{self:?}")


I agree it would be great to have a special type signature class display

Maybe something like (any int) or Integer or (any timestamp) for Timestamp 🤔

alamb · 2024-12-09T15:56:58Z

datafusion/expr-common/src/signature.rs

+                .map(|logical_type| match logical_type {
+                    TypeSignatureClass::Native(l) => get_data_types(l.native()),
+                    TypeSignatureClass::Timestamp => {
+                        vec![DataType::Timestamp(TimeUnit::Nanosecond, None)]


I think this should use WILDCARD to match any timestamp (not just timestamps without timezones)

This is actually used for displaying the signature information for error message.

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.

vec![DataType::Timestamp(TimeUnit::Nanosecond, None), DataType::Timestamp(TimeUnit::Nanosecond, Some(TIMEZONE_WILDCARD.into()))]

alamb · 2024-12-09T15:57:19Z

datafusion/common/src/types/native.rs

@@ -245,6 +245,8 @@ impl LogicalType for NativeType {
            (Self::FixedSizeBinary(size), _) => FixedSizeBinary(*size),
            (Self::String, LargeBinary) => LargeUtf8,
            (Self::String, BinaryView) => Utf8View,
+            // We don't cast to another kind of string type if the origin one is already a string type


alamb · 2024-12-09T15:58:42Z

datafusion/sqllogictest/test_files/expr.slt

-
-query I
+# Second argument should not be string, failed in postgres too.
+query error


this feels like a regression to me (even if it fails in postgres). I think automatically coercing to a string is important for our users in InfluxDB.

…trings. Update SQL logic tests to reflect changes in timestamp handling, including expected outputs for queries involving nanoseconds and seconds.

jayzhan211 · 2024-12-10T12:17:49Z

datafusion/expr/src/type_coercion/functions.rs

+                    TypeSignatureClass::Timestamp if logical_type == NativeType::String => {
+                        Ok(DataType::Timestamp(TimeUnit::Nanosecond, None))
+                    }
+                    TypeSignatureClass::Timestamp if logical_type.is_timestamp() => {


timestamp is matched whatever the timezone it has

…d maintainability. Update the `TypeSignatureClass` documentation to clarify its purpose in function signatures, particularly regarding coercible types. This change enhances the handling of implicit casting from strings to timestamps.

…ctions. Updated expected outputs for `date_part` and `extract` functions to reflect proper behavior with nanoseconds and seconds. This change improves the accuracy of test cases in the `expr.slt` file.

…fication. Updated the logic to include an additional DataType for timestamps with a timezone wildcard, improving flexibility in timestamp operations.

alamb · 2024-12-11T14:52:52Z

Here is a PR with some more tests: Add tests for date_part on columns + timestamps with / without timezones #13732

alamb

Thank you @jayzhan211 -- I know this has been a long process. Sorry for the slower pace but I think as DataFusion attempts to be more stable, more attention is warranted for potential changes.

Other than the string coercion (I left a question) this PR looks good to me given your comment on #13732 (comment)

❤️ Thanks again

alamb · 2024-12-12T13:51:16Z

datafusion/sqllogictest/test_files/expr.slt

 query I
 SELECT date_part('second', '2020-09-08T12:00:12.12345678+00:00')
 ----
 12

 query I
-SELECT date_part('millisecond', '2020-09-08T12:00:12.12345678+00:00')
+SELECT date_part('second', timestamp '2020-09-08T12:00:12.12345678+00:00')


are these test changes still needed?

From my perspective as long as this PR doesn't change the ability to use a string as an argument to a function that requires a timestamp, it is a good change to me

I haven't merge the code from the main

jayzhan211 · 2024-12-12T14:12:05Z

Thank you @jayzhan211 -- I know this has been a long process. Sorry for the slower pace but I think as DataFusion attempts to be more stable, more attention is warranted for potential changes.

Other than the string coercion (I left a question) this PR looks good to me given your comment on #13732 (comment)

❤️ Thanks again

I'm also thinking about the comment above about whether to differentiate timestamp timezone or not. Given that we already have NativeType::Timestamp to differentiate with tz or without cases, I still don't think it is a good idea to add another abstraction in TypeSignatureClass. For date_bin that takes 2 args, I think using TypeSignatureClass::Native() might be a better idea. We can review the design again when we deal with date_bin later on

…nes (#13732) * Add tests for date_part on columns + timestamps with / without timezones * Add tests from #13372 * remove trailing whitespace

… not_impl_err import for better error handling

alamb

Love it -- thank you @jayzhan211 for pushing this forward and thank you for putting up with the long review cycle

alamb · 2024-12-13T18:09:12Z

datafusion/sqllogictest/test_files/expr.slt

@@ -560,7 +560,7 @@ select repeat('-1.2', arrow_cast(3, 'Int32'));
 ----


Lack of test changes look great to me ❤️

jayzhan211 · 2024-12-14T01:36:33Z

Thanks @alamb @goldmedal @findepi

jayzhan211 added 5 commits November 12, 2024 13:33

add type sig class

2ea2c27

Signed-off-by: jayzhan211 <[email protected]>

timestamp

664edaa

Signed-off-by: jayzhan211 <[email protected]>

date part

fc99216

Signed-off-by: jayzhan211 <[email protected]>

fmt

2eac9f8

Signed-off-by: jayzhan211 <[email protected]>

taplo format

dd3fb7f

Signed-off-by: jayzhan211 <[email protected]>

github-actions bot added logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) common Related to common crate functions labels Nov 12, 2024

jayzhan211 added 4 commits November 12, 2024 16:33

tpch test

e114c86

Signed-off-by: jayzhan211 <[email protected]>

msrc issue

f04aed5

Signed-off-by: jayzhan211 <[email protected]>

msrc issue

3b8030c

Signed-off-by: jayzhan211 <[email protected]>

explicit hash

6b1e08a

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 marked this pull request as ready for review November 12, 2024 11:08

Merge branch 'main' of github.com:apache/datafusion into type-class

afa23df

findepi approved these changes Nov 16, 2024

View reviewed changes

jayzhan211 marked this pull request as draft November 23, 2024 06:15

jayzhan211 requested a review from goldmedal December 9, 2024 13:35

jayzhan211 commented Dec 9, 2024

View reviewed changes

alamb reviewed Dec 9, 2024

View reviewed changes

Enhance type coercion for timestamps to allow implicit casting from s…

f141e89

…trings. Update SQL logic tests to reflect changes in timestamp handling, including expected outputs for queries involving nanoseconds and seconds.

jayzhan211 commented Dec 10, 2024

View reviewed changes

jayzhan211 marked this pull request as draft December 10, 2024 14:33

jayzhan211 added 2 commits December 11, 2024 09:56

Fix SQL logic tests to correct query error handling for timestamp fun…

7920421

…ctions. Updated expected outputs for `date_part` and `extract` functions to reflect proper behavior with nanoseconds and seconds. This change improves the accuracy of test cases in the `expr.slt` file.

Enhance timestamp handling in TypeSignature to support timezone speci…

4a7404d

…fication. Updated the logic to include an additional DataType for timestamps with a timezone wildcard, improving flexibility in timestamp operations.

jayzhan211 marked this pull request as ready for review December 11, 2024 12:05

alamb mentioned this pull request Dec 11, 2024

minor: Extract tests for EXTRACT AND date_part to their own file #13731

Merged

alamb added a commit to alamb/datafusion that referenced this pull request Dec 11, 2024

Add tests from apache#13372

a151885

alamb added a commit to alamb/datafusion that referenced this pull request Dec 11, 2024

Add tests from apache#13372

5c8de1f

alamb mentioned this pull request Dec 11, 2024

Add tests for date_part on columns + timestamps with / without timezones #13732

Merged

Merge branch 'main' of github.com:apache/datafusion into type-class

7400429

jayzhan211 marked this pull request as draft December 12, 2024 13:48

alamb reviewed Dec 12, 2024

View reviewed changes

jayzhan211 marked this pull request as ready for review December 12, 2024 14:13

alamb added a commit that referenced this pull request Dec 12, 2024

Add tests for date_part on columns + timestamps with / without timezo…

aeddbd9

…nes (#13732) * Add tests for date_part on columns + timestamps with / without timezones * Add tests from #13372 * remove trailing whitespace

Merge branch 'main' of github.com:apache/datafusion into type-class

3647bb5

jayzhan211 marked this pull request as draft December 13, 2024 10:05

Refactor date_part function: remove redundant imports and add missing…

830b2d5

… not_impl_err import for better error handling

jayzhan211 marked this pull request as ready for review December 13, 2024 14:15

alamb approved these changes Dec 13, 2024

View reviewed changes

alamb changed the title ~~TypeSignatureClass for mixed type function signature~~ Simplify type signatures using TypeSignatureClass for mixed type function signature Dec 13, 2024

jayzhan211 merged commit 08d3b65 into apache:main Dec 14, 2024
28 checks passed

jayzhan211 deleted the type-class branch December 14, 2024 01:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify type signatures using `TypeSignatureClass` for mixed type function signature #13372

Simplify type signatures using `TypeSignatureClass` for mixed type function signature #13372

jayzhan211 commented Nov 12, 2024 •

edited

Loading

findepi Nov 16, 2024

jayzhan211 Nov 17, 2024

alamb Dec 9, 2024

goldmedal Dec 10, 2024

jayzhan211 Dec 11, 2024

goldmedal Dec 11, 2024

findepi Dec 11, 2024

jayzhan211 Dec 12, 2024 •

edited

Loading

goldmedal Dec 12, 2024

findepi Nov 16, 2024

findepi Nov 16, 2024

jayzhan211 Nov 17, 2024

findepi Nov 16, 2024

alamb Dec 9, 2024

alamb commented Nov 20, 2024

jayzhan211 commented Nov 21, 2024 •

edited

Loading

jayzhan211 Dec 9, 2024

alamb left a comment

alamb Dec 9, 2024

alamb Dec 9, 2024

alamb Dec 9, 2024

jayzhan211 Dec 10, 2024

goldmedal Dec 10, 2024

alamb Dec 9, 2024

alamb Dec 9, 2024

jayzhan211 Dec 10, 2024

alamb commented Dec 11, 2024

alamb left a comment

alamb Dec 12, 2024

jayzhan211 Dec 12, 2024

jayzhan211 commented Dec 12, 2024 •

edited

Loading

alamb left a comment

alamb Dec 13, 2024

jayzhan211 commented Dec 14, 2024

	Exact(vec![
	DataType::Interval(MonthDayNano),
	Timestamp(array_type, None),
	Timestamp(Nanosecond, None),
	]),
	Exact(vec![
	DataType::Interval(MonthDayNano),
	Timestamp(array_type, Some(TIMEZONE_WILDCARD.into())),
	Timestamp(Nanosecond, Some(TIMEZONE_WILDCARD.into())),
	]),

		@@ -560,7 +560,7 @@ select repeat('-1.2', arrow_cast(3, 'Int32'));
		----

Simplify type signatures using TypeSignatureClass for mixed type function signature #13372

Simplify type signatures using TypeSignatureClass for mixed type function signature #13372

Conversation

jayzhan211 commented Nov 12, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Postgres

DuckDB

Choose a reason for hiding this comment

jayzhan211 Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Nov 20, 2024

jayzhan211 commented Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Dec 11, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 commented Dec 12, 2024 • edited Loading

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 commented Dec 14, 2024

Simplify type signatures using `TypeSignatureClass` for mixed type function signature #13372

Simplify type signatures using `TypeSignatureClass` for mixed type function signature #13372

jayzhan211 commented Nov 12, 2024 •

edited

Loading

jayzhan211 Dec 12, 2024 •

edited

Loading

jayzhan211 commented Nov 21, 2024 •

edited

Loading

jayzhan211 commented Dec 12, 2024 •

edited

Loading