Skip to content
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

Supporting casts to timestamptz: Eq Utf8 of binary physical should be same" #5164

Closed
Tracked by #3148
alamb opened this issue Feb 2, 2023 · 6 comments · Fixed by #5777
Closed
Tracked by #3148

Supporting casts to timestamptz: Eq Utf8 of binary physical should be same" #5164

alamb opened this issue Feb 2, 2023 · 6 comments · Fixed by #5777
Assignees
Labels
bug Something isn't working

Comments

@alamb
Copy link
Contributor

alamb commented Feb 2, 2023

Describe the bug
After #5140 and #5117 we can cast strings to Timestamp without a timezone. However, casting with timezone is not working yet, as noticed by @waitingkuo

To Reproduce
In datafusion-cli:

select '2000-01-01T00:00:00'::timestamp::timestamptz = '2000-01-01T00:00:00';
Internal("The type of Timestamp(Nanosecond, Some(\"+00:00\")) Eq Utf8 of binary physical should be same")
❯ select '2000-01-01T00:00:00'::timestamp = '2000-01-01T00:00:00';
+-----------------------------------------------------------+
| Utf8("2000-01-01T00:00:00") = Utf8("2000-01-01T00:00:00") |
+-----------------------------------------------------------+
| true                                                      |
+-----------------------------------------------------------+
1 row in set. Query took 0.004 seconds.
❯ select '2000-01-01T00:00:00'::timestamptz = '2000-01-01T00:00:00';
NotImplemented("Unsupported CAST from Utf8 to Timestamp(Nanosecond, Some(\"+00:00\"))")

Expected behavior
I expect all three queries to run and return true

Additional context
Add any other context about the problem here.

@alamb alamb added the bug Something isn't working label Feb 2, 2023
@comphead
Copy link
Contributor

comphead commented Feb 2, 2023

@alamb I'll take it if noone else will. Need to finish this off

@comphead
Copy link
Contributor

comphead commented Feb 5, 2023

@alamb @waitingkuo likely we have some design issues on TimestampTz

please consider such simple case

        let valid = StringArray::from(vec![
            "2023-01-01 04:05:06.789-8",
            "2023-01-01 04:05:06.789-7",
        ]);

        let array = Arc::new(valid) as ArrayRef;
        let b = cast(&array, &DataType::Timestamp(TimeUnit::Nanosecond,?????????)).unwrap();

Ive put ???????? as its not really clear what has to be there, Some("+07:00") or Some("+08:00")? cast works for array which can contain mixed timezones, however the output datatype is a single value with only 1 supported tz

@alamb
Copy link
Contributor Author

alamb commented Feb 6, 2023

In terms of ???????? I would expect that one could put any None, Some("+07:00") or Some("+08:00") and the resulting timestamps would be adjusted on input to reflect what timezone they appeared in

cc @tustvold in case he has additional thoughts

@tustvold
Copy link
Contributor

tustvold commented Feb 6, 2023

cc @tustvold in case he has additional thoughts

I agree, we should adjust the timestamp from the timezone in the string, if any, to time since UTC epoch, and store this in the value. The timezone of the output can then be the DataFusion default, unless otherwise specified, regardless this doesn't impact the encoded value which is always time since UTC epoch

@comphead
Copy link
Contributor

Waiting arrow 34.0.0

@comphead
Copy link
Contributor

DataFusion CLI v21.0.0
❯ select '2000-01-01T00:00:00'::timestamp::timestamptz = '2000-01-01T00:00:00';

+-----------------------------------------------------------+
| Utf8("2000-01-01T00:00:00") = Utf8("2000-01-01T00:00:00") |
+-----------------------------------------------------------+
| true                                                      |
+-----------------------------------------------------------+
1 row in set. Query took 0.006 seconds.
❯ 

That finally works after Arrow 36.0.0, I'll create a PR with tests
@alamb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants