arrow_cast to cast to timestamptz #5914

waitingkuo · 2023-04-07T11:45:18Z

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

allow arrow_cast to cast to timestamptz

❯ select arrow_cast(timestamp '2000-01-01T00:00:00', 'Timestamp(Nanosecond, Some( "+08:00" ))');
+-----------------------------+
| Utf8("2000-01-01T00:00:00") |
+-----------------------------+
| 2000-01-01T08:00:00+08:00   |
+-----------------------------+
1 row in set. Query took 0.010 seconds.

❯ select arrow_cast(timestamptz '2000-01-01T00:00:00Z', 'Timestamp(Nanosecond, Some( "+08:00" ))');
+------------------------------+
| Utf8("2000-01-01T00:00:00Z") |
+------------------------------+
| 2000-01-01T08:00:00+08:00    |
+------------------------------+
1 row in set. Query took 0.002 seconds.

❯ select arrow_cast(0, 'Timestamp(Nanosecond, Some( "+08:00" ))');
+---------------------------+
| Int64(0)                  |
+---------------------------+
| 1970-01-01T08:00:00+08:00 |
+---------------------------+
1 row in set. Query took 0.004 seconds.

Are these changes tested?

yes

Are there any user-facing changes?

waitingkuo · 2023-04-07T11:51:27Z

datafusion/sql/src/expr/arrow_cast.rs

+    fn parse_timezone(&mut self, context: &str) -> Result<Option<String>> {
+        match self.next_token()? {
+            Token::None => Ok(None),
+            Token::Some => {
+                self.expect_token(Token::LParen)?;
+                let timezone = self.parse_double_quoted_string("Timezone")?;
+                self.expect_token(Token::RParen)?;
+                Ok(Some(timezone))
+            }
+            tok => Err(make_error(
+                self.val,
+                &format!("finding Timezone for {context}, got {tok}"),
+            )),
+        }
+    }


add a parser to parse timezone Option<String>
either None, or Some("SomeTimezone")

waitingkuo · 2023-04-07T11:51:55Z

datafusion/sql/src/expr/arrow_cast.rs

+    /// Parses the next double quoted string
+    fn parse_double_quoted_string(&mut self, context: &str) -> Result<String> {
+        match self.next_token()? {
+            Token::DoubleQuotedString(s) => Ok(s),
+            tok => Err(make_error(
+                self.val,
+                &format!("finding double quoted string for {context}, got '{tok}'"),
+            )),
+        }
+    }
+


add parser to parse the double quoted string e.g. "+00:00"

tustvold · 2023-04-07T12:02:24Z

❯ select arrow_cast(timestamp '2000-01-01T00:00:00', 'Timestamp(Nanosecond, Some( "+08:00" ))');
+-----------------------------+
| Utf8("2000-01-01T00:00:00") |
+-----------------------------+
| 2000-01-01T08:00:00+08:00   |
+-----------------------------+
1 row in set. Query took 0.010 seconds.

IMO this behaviour is a bug in arrow-cast that should be fixed, I'm not sure if we wish to expose this.

The "correct" behaviour would be

❯ select arrow_cast(timestamp '2000-01-01T00:00:00', 'Timestamp(Nanosecond, Some( "+08:00" ))');
+-----------------------------+
| Utf8("2000-01-01T00:00:00") |
+-----------------------------+
| 2000-01-01T00:00:00+08:00   |
+-----------------------------+
1 row in set. Query took 0.010 seconds.

alamb

Looks great @waitingkuo -- I had some small suggestions on some other tests but we can also add them as a follow on PR

alamb · 2023-04-07T12:02:03Z

datafusion/sql/src/expr/arrow_cast.rs

+            DataType::Timestamp(TimeUnit::Nanosecond, Some("+08:00".into())),
+            DataType::Timestamp(TimeUnit::Microsecond, Some("+08:00".into())),
+            DataType::Timestamp(TimeUnit::Millisecond, Some("+08:00".into())),
+            DataType::Timestamp(TimeUnit::Second, Some("+08:00".into())),


Some other test cases that would be good:

"" (empty timezone)

Also errors:

Three double quotes

"Timestamp(Nanosecond, Some("+00:00""))"

thanks @alamb , more error handling and test cases added

tustvold · 2023-04-07T13:11:13Z

apache/arrow-rs#4034 should fix the timestamp handling in arrow-cast, if we are quick it may still make the release this afternoon

waitingkuo · 2023-04-07T13:13:04Z

datafusion/sql/src/expr/arrow_cast.rs

+                r#"Timestamp(Nanosecond, Some(+00:00))"#,
+                "Error unrecognized word: +00:00",
+            ),
+            (
+                r#"Timestamp(Nanosecond, Some("+00:00))"#,
+                r#"parsing "+00:00 as double quoted string: last char must be ""#,
+            ),
+            (
+                r#"Timestamp(Nanosecond, Some(""))"#,
+                r#"parsing "" as double quoted string: empty string isn't supported"#,
+            ),
+            (
+                r#"Timestamp(Nanosecond, Some("+00:00""))"#,
+                r#"parsing "+00:00"" as double quoted string: escaped double quote isn't supported"#,


@alamb
added error cases for
empty string and
double quoted string that contains double quote

waitingkuo · 2023-04-07T13:25:24Z

datafusion/core/tests/sqllogictests/test_files/arrow_typeof.slt

+# once https://github.com/apache/arrow-rs/issues/1936 fixed, please update this query
+query P
+select arrow_cast(timestamp '2000-01-01T00:00:00Z', 'Timestamp(Nanosecond, Some( "+08:00" ))');
+----
+2000-01-01T08:00:00+08:00


@tustvold thank you
added this case to notify users once the kernel casting issue fixed

alamb

Love it -- thanks again @waitingkuo

alamb · 2023-04-07T14:20:21Z

datafusion/sql/src/expr/arrow_cast.rs

+                    }
+                }
+
+                if len == 2 {


👍 nice error messages

arrow_cast to cast to timestamptz

cad1b77

github-actions bot added core Core DataFusion crate sql SQL Planner sqllogictest SQL Logic Tests (.slt) labels Apr 7, 2023

waitingkuo commented Apr 7, 2023

View reviewed changes

alamb approved these changes Apr 7, 2023

View reviewed changes

add more error handling for parsing double quoted string

4eb4d53

waitingkuo commented Apr 7, 2023

View reviewed changes

waitingkuo added 2 commits April 7, 2023 21:14

git fmt

341f5bc

add test cases in .slt

a926ee6

waitingkuo commented Apr 7, 2023

View reviewed changes

cargo clippy

56a7b9e

alamb approved these changes Apr 7, 2023

View reviewed changes

datafusion/sql/src/expr/arrow_cast.rs

}

}

if len == 2 {

Copy link

Contributor

alamb Apr 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 nice error messages

alamb merged commit a0fb3c6 into apache:main Apr 7, 2023

tustvold mentioned this pull request Apr 11, 2023

Incorrect Scalar Timezone Coercion #5952

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

arrow_cast to cast to timestamptz #5914

arrow_cast to cast to timestamptz #5914

waitingkuo commented Apr 7, 2023

waitingkuo Apr 7, 2023

waitingkuo Apr 7, 2023

tustvold commented Apr 7, 2023 •

edited

Loading

alamb left a comment

alamb Apr 7, 2023

waitingkuo Apr 7, 2023

tustvold commented Apr 7, 2023

waitingkuo Apr 7, 2023

waitingkuo Apr 7, 2023

alamb left a comment

alamb Apr 7, 2023

arrow_cast to cast to timestamptz #5914

arrow_cast to cast to timestamptz #5914

Conversation

waitingkuo commented Apr 7, 2023

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?

waitingkuo Apr 7, 2023

Choose a reason for hiding this comment

waitingkuo Apr 7, 2023

Choose a reason for hiding this comment

tustvold commented Apr 7, 2023 • edited Loading

alamb left a comment

Choose a reason for hiding this comment

alamb Apr 7, 2023

Choose a reason for hiding this comment

waitingkuo Apr 7, 2023

Choose a reason for hiding this comment

tustvold commented Apr 7, 2023

waitingkuo Apr 7, 2023

Choose a reason for hiding this comment

waitingkuo Apr 7, 2023

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Apr 7, 2023

Choose a reason for hiding this comment

tustvold commented Apr 7, 2023 •

edited

Loading