Fix Presto's format_datetime function with time zone #11283

kevinwilfong · 2024-10-16T22:06:32Z

Summary:
The format_datetime currently outptus the time zone id if for 3 or fewer 'z' characters
in the format string. However, the JODA library, which this is based on, does this for
3 or more 'Z' characters.

https://www.joda.org/joda-time/apidocs/org/joda/time/format/DateTimeFormat.html

This diff fixes this, as well as adds support for a single 'Z' (which outputs the same
thing as 'ZZ' just without the colon). So 'Z' is fully supported for any number of
characters.

Differential Revision: D64500193

facebook-github-bot · 2024-10-16T22:06:42Z

This pull request was exported from Phabricator. Differential Revision: D64500193

netlify · 2024-10-16T22:06:48Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`68a6d39`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/671fc4b201198800081d9f47

bikramSingh91

just a small nit about the PR summary, do you mind including this line from JODA docs (with a small modification that adds examples) in there apart from the link

'Z' outputs offset without a colon (-0800), 'ZZ' outputs the offset with a colon(-08:00), 'ZZZ' or more outputs the zone id(America/Los_Angeles).

, and then explain which parts are out of sync which the PR is fixing.

bikramSingh91 · 2024-10-21T18:59:58Z

velox/functions/lib/DateTimeFormatter.cpp

-        }
-        size += std::max(
-            token.pattern.minRepresentDigits, timezone->name().length());
+        VELOX_UNSUPPORTED(


can you please add a test case for lower case z to test this error condition

bikramSingh91 · 2024-10-21T19:00:18Z

velox/functions/lib/DateTimeFormatter.cpp

+          if (timezone == nullptr) {
+            VELOX_USER_FAIL("Timezone unknown");
+          }
+          size += std::max(


just curious, is this always true? like if timezone->name().length() is less than token.pattern.minRepresentDigits will the remaining characters be filled up with something? or does it not matter because we are only calculating the "MAX-ResultSize" here and the actual can be smaller?

that's a good point, it won't matter because it's just reserving a buffer, but I tested in Presto Java and minRepresentDigits doesn't have any bearing on the string we print out, I'll fix this.

bikramSingh91 · 2024-10-21T19:01:09Z

velox/functions/lib/DateTimeFormatter.cpp

-          const auto& piece = timezone->name();
-          std::memcpy(result, piece.data(), piece.length());
-          result += piece.length();
+          VELOX_UNSUPPORTED("time zone name is not yet supported");
        } break;

        case DateTimeFormatSpecifier::TIMEZONE_OFFSET_ID: {
          // Zone: 'Z' outputs offset without a colon, 'ZZ' outputs the offset
          // with a colon, 'ZZZ' or more outputs the zone id.
          // TODO Add support for 'Z' and 'ZZZ'.


nit: can update this comment to remove the TODO now

bikramSingh91 · 2024-10-21T19:01:28Z

velox/functions/lib/DateTimeFormatter.cpp

-          result += appendTimezoneOffset(offset, result);
+
+          if (token.pattern.minRepresentDigits >= 3) {
+            const auto& piece = timezone->name();


nit: can you please add an inline comment, append timezoneID

bikramSingh91 · 2024-10-21T19:01:51Z

velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp

@@ -3217,8 +3217,9 @@ TEST_F(DateTimeFunctionsTest, formatDateTime) {
  // Time zone test cases - 'z'
  setQueryTimeZone("Asia/Kolkata");
  EXPECT_EQ(
-      "Asia/Kolkata", formatDatetime(parseTimestamp("1970-01-01"), "zzzz"));
+      "Asia/Kolkata", formatDatetime(parseTimestamp("1970-01-01"), "ZZZ"));


can you please add a test case for 4 or more Zs

…or#11283) Summary: The format_datetime currently outptus the time zone id if for 3 or fewer 'z' characters in the format string. However, the JODA library, which this is based on, does this for 3 or more 'Z' characters. https://www.joda.org/joda-time/apidocs/org/joda/time/format/DateTimeFormat.html This diff fixes this, as well as adds support for a single 'Z' (which outputs the same thing as 'ZZ' just without the colon). So 'Z' is fully supported for any number of characters. Differential Revision: D64500193

…or#11283) Summary: The format_datetime currently outptus the time zone id if for 3 or fewer 'z' characters in the format string. However, the JODA library, which this is based on, does this for 3 or more 'Z' characters. This diff fixes this, as well as adds support for a single 'Z' (which outputs the same thing as 'ZZ' just without the colon). So 'Z' is fully supported for any number of characters. To be more explicit: https://www.joda.org/joda-time/apidocs/org/joda/time/format/DateTimeFormat.html From the JODA docs: `'Z' outputs offset without a colon (-0800), 'ZZ' outputs the offset with a colon(-08:00), 'ZZZ' or more outputs the zone id(America/Los_Angeles).` And not clearly explained in the docs, but from experimentation: `'z', 'zz', or 'zzz' outputs the abbreviation of the time zone(PDT), and 'zzzz' or more outputs the time zone name(Pacific Daylight Time)` Currently DateTimeFormatter treats 'zzzz' or more like JODA treats 'ZZZ' or more. This diff marks 'zzzz' or more as unsupported (we can implement that in a future change), and moves that logic under 'ZZZ' or more to be consistent. It also implements 'Z' (previously only 'ZZ' was implemented in DateTimeFormatter). Differential Revision: D64500193

facebook-github-bot · 2024-10-21T21:49:24Z

This pull request was exported from Phabricator. Differential Revision: D64500193

…or#11283) Summary: Pull Request resolved: facebookincubator#11283 The format_datetime currently outptus the time zone id if for 3 or fewer 'z' characters in the format string. However, the JODA library, which this is based on, does this for 3 or more 'Z' characters. https://www.joda.org/joda-time/apidocs/org/joda/time/format/DateTimeFormat.html This diff fixes this, as well as adds support for a single 'Z' (which outputs the same thing as 'ZZ' just without the colon). So 'Z' is fully supported for any number of characters. Differential Revision: D64500193

…or#11283) Summary: The format_datetime currently outptus the time zone id if for 3 or fewer 'z' characters in the format string. However, the JODA library, which this is based on, does this for 3 or more 'Z' characters. This diff fixes this, as well as adds support for a single 'Z' (which outputs the same thing as 'ZZ' just without the colon). So 'Z' is fully supported for any number of characters. To be more explicit: https://www.joda.org/joda-time/apidocs/org/joda/time/format/DateTimeFormat.html From the JODA docs: `'Z' outputs offset without a colon (-0800), 'ZZ' outputs the offset with a colon(-08:00), 'ZZZ' or more outputs the zone id(America/Los_Angeles).` And not clearly explained in the docs, but from experimentation: `'z', 'zz', or 'zzz' outputs the abbreviation of the time zone(PDT), and 'zzzz' or more outputs the time zone name(Pacific Daylight Time)` Currently DateTimeFormatter treats 'zzzz' or more like JODA treats 'ZZZ' or more. This diff marks 'zzzz' or more as unsupported (we can implement that in a future change), and moves that logic under 'ZZZ' or more to be consistent. It also implements 'Z' (previously only 'ZZ' was implemented in DateTimeFormatter). Reviewed By: bikramSingh91 Differential Revision: D64500193

facebook-github-bot · 2024-10-24T21:33:01Z

This pull request was exported from Phabricator. Differential Revision: D64500193

…or#11283) Summary: Pull Request resolved: facebookincubator#11283 The format_datetime currently outptus the time zone id if for 3 or fewer 'z' characters in the format string. However, the JODA library, which this is based on, does this for 3 or more 'Z' characters. https://www.joda.org/joda-time/apidocs/org/joda/time/format/DateTimeFormat.html This diff fixes this, as well as adds support for a single 'Z' (which outputs the same thing as 'ZZ' just without the colon). So 'Z' is fully supported for any number of characters. Differential Revision: D64500193

…or#11283) Summary: The format_datetime currently outptus the time zone id if for 3 or fewer 'z' characters in the format string. However, the JODA library, which this is based on, does this for 3 or more 'Z' characters. This diff fixes this, as well as adds support for a single 'Z' (which outputs the same thing as 'ZZ' just without the colon). So 'Z' is fully supported for any number of characters. To be more explicit: https://www.joda.org/joda-time/apidocs/org/joda/time/format/DateTimeFormat.html From the JODA docs: `'Z' outputs offset without a colon (-0800), 'ZZ' outputs the offset with a colon(-08:00), 'ZZZ' or more outputs the zone id(America/Los_Angeles).` And not clearly explained in the docs, but from experimentation: `'z', 'zz', or 'zzz' outputs the abbreviation of the time zone(PDT), and 'zzzz' or more outputs the time zone name(Pacific Daylight Time)` Currently DateTimeFormatter treats 'zzzz' or more like JODA treats 'ZZZ' or more. This diff marks 'zzzz' or more as unsupported (we can implement that in a future change), and moves that logic under 'ZZZ' or more to be consistent. It also implements 'Z' (previously only 'ZZ' was implemented in DateTimeFormatter). Reviewed By: bikramSingh91 Differential Revision: D64500193

facebook-github-bot · 2024-10-28T17:07:14Z

This pull request was exported from Phabricator. Differential Revision: D64500193

…or#11283) Summary: The format_datetime currently outptus the time zone id if for 3 or fewer 'z' characters in the format string. However, the JODA library, which this is based on, does this for 3 or more 'Z' characters. This diff fixes this, as well as adds support for a single 'Z' (which outputs the same thing as 'ZZ' just without the colon). So 'Z' is fully supported for any number of characters. To be more explicit: https://www.joda.org/joda-time/apidocs/org/joda/time/format/DateTimeFormat.html From the JODA docs: `'Z' outputs offset without a colon (-0800), 'ZZ' outputs the offset with a colon(-08:00), 'ZZZ' or more outputs the zone id(America/Los_Angeles).` And not clearly explained in the docs, but from experimentation: `'z', 'zz', or 'zzz' outputs the abbreviation of the time zone(PDT), and 'zzzz' or more outputs the time zone name(Pacific Daylight Time)` Currently DateTimeFormatter treats 'zzzz' or more like JODA treats 'ZZZ' or more. This diff marks 'zzzz' or more as unsupported (we can implement that in a future change), and moves that logic under 'ZZZ' or more to be consistent. It also implements 'Z' (previously only 'ZZ' was implemented in DateTimeFormatter). Reviewed By: bikramSingh91 Differential Revision: D64500193

…or#11283) Summary: Pull Request resolved: facebookincubator#11283 The format_datetime currently outptus the time zone id if for 3 or fewer 'z' characters in the format string. However, the JODA library, which this is based on, does this for 3 or more 'Z' characters. https://www.joda.org/joda-time/apidocs/org/joda/time/format/DateTimeFormat.html This diff fixes this, as well as adds support for a single 'Z' (which outputs the same thing as 'ZZ' just without the colon). So 'Z' is fully supported for any number of characters. Differential Revision: D64500193

facebook-github-bot · 2024-10-29T19:54:41Z

This pull request has been merged in d940434.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 16, 2024

facebook-github-bot added the fb-exported label Oct 16, 2024

bikramSingh91 reviewed Oct 21, 2024

View reviewed changes

kevinwilfong force-pushed the export-D64500193 branch from 6a0e062 to 31390d0 Compare October 21, 2024 21:49

bikramSingh91 approved these changes Oct 22, 2024

View reviewed changes

kevinwilfong force-pushed the export-D64500193 branch from 31390d0 to c35ca80 Compare October 24, 2024 21:32

kevinwilfong force-pushed the export-D64500193 branch from c35ca80 to 68a6d39 Compare October 28, 2024 17:06

facebook-github-bot closed this in d940434 Oct 29, 2024

facebook-github-bot added the Merged label Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Presto's format_datetime function with time zone #11283

Fix Presto's format_datetime function with time zone #11283

kevinwilfong commented Oct 16, 2024

facebook-github-bot commented Oct 16, 2024

netlify bot commented Oct 16, 2024 •

edited

Loading

bikramSingh91 left a comment

bikramSingh91 Oct 21, 2024

bikramSingh91 Oct 21, 2024

kevinwilfong Oct 21, 2024

bikramSingh91 Oct 21, 2024

bikramSingh91 Oct 21, 2024

bikramSingh91 Oct 21, 2024

facebook-github-bot commented Oct 21, 2024

facebook-github-bot commented Oct 24, 2024

facebook-github-bot commented Oct 28, 2024

facebook-github-bot commented Oct 29, 2024

Fix Presto's format_datetime function with time zone #11283

Fix Presto's format_datetime function with time zone #11283

Conversation

kevinwilfong commented Oct 16, 2024

facebook-github-bot commented Oct 16, 2024

netlify bot commented Oct 16, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

bikramSingh91 left a comment

Choose a reason for hiding this comment

bikramSingh91 Oct 21, 2024

Choose a reason for hiding this comment

bikramSingh91 Oct 21, 2024

Choose a reason for hiding this comment

kevinwilfong Oct 21, 2024

Choose a reason for hiding this comment

bikramSingh91 Oct 21, 2024

Choose a reason for hiding this comment

bikramSingh91 Oct 21, 2024

Choose a reason for hiding this comment

bikramSingh91 Oct 21, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Oct 21, 2024

facebook-github-bot commented Oct 24, 2024

facebook-github-bot commented Oct 28, 2024

facebook-github-bot commented Oct 29, 2024

netlify bot commented Oct 16, 2024 •

edited

Loading