Skip to content

Commit

Permalink
Fix Presto's format_datetime function with time zone (facebookincubat…
Browse files Browse the repository at this point in the history 
…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
  • Loading branch information
@facebook-github-bot
Kevin Wilfong authored and facebook-github-bot committed Oct 21, 2024
1 parent 0e5f3b0 commit c89af93
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 34 deletions.
59 changes: 29 additions & 30 deletions velox/functions/lib/DateTimeFormatter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ std::string formatFractionOfSecond(
return toAdd;
}

int32_t appendTimezoneOffset(int64_t offset, char* result) {
int32_t appendTimezoneOffset(int64_t offset, char* result, bool includeColon) {
int pos = 0;
if (offset >= 0) {
result[pos++] = '+';
Expand All @@ -531,7 +531,9 @@ int32_t appendTimezoneOffset(int64_t offset, char* result) {
result[pos++] = char(hours % 10 + '0');
}

result[pos++] = ':';
if (includeColon) {
result[pos++] = ':';
}

const auto minutes = (offset / 60) % 60;
if LIKELY (minutes == 0) {
Expand Down Expand Up @@ -1036,20 +1038,24 @@ uint32_t DateTimeFormatter::maxResultSize(const tz::TimeZone* timezone) const {
size += std::max((int)token.pattern.minRepresentDigits, 9);
break;
case DateTimeFormatSpecifier::TIMEZONE:
if (timezone == nullptr) {
VELOX_USER_FAIL("Timezone unknown");
}
size += std::max(
token.pattern.minRepresentDigits, timezone->name().length());
VELOX_UNSUPPORTED(
"Date format specifier is not supported: {} ({})",
getSpecifierName(token.pattern.specifier),
token.pattern.minRepresentDigits);

break;
case DateTimeFormatSpecifier::TIMEZONE_OFFSET_ID:
if (token.pattern.minRepresentDigits != 2) {
VELOX_UNSUPPORTED(
"Date format specifier is not supported: {} ({})",
getSpecifierName(token.pattern.specifier),
token.pattern.minRepresentDigits);
if (token.pattern.minRepresentDigits == 1) {
size += 8;
} else if (token.pattern.minRepresentDigits == 2) {
size += 9;
} else {
if (timezone == nullptr) {
VELOX_USER_FAIL("Timezone unknown");
}
size += std::max(
token.pattern.minRepresentDigits, timezone->name().length());
}
size += 9;
break;
// Not supported.
case DateTimeFormatSpecifier::WEEK_YEAR:
Expand Down Expand Up @@ -1280,35 +1286,28 @@ int32_t DateTimeFormatter::format(
case DateTimeFormatSpecifier::TIMEZONE: {
// TODO: implement short name time zone, need a map from full name to
// short name
if (token.pattern.minRepresentDigits <= 3) {
VELOX_UNSUPPORTED("short name time zone is not yet supported");
}
if (timezone == nullptr) {
VELOX_USER_FAIL("Timezone unknown");
}
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'.
if (token.pattern.minRepresentDigits != 2) {
VELOX_UNSUPPORTED(
"format is not supported for specifier {} ({})",
getSpecifierName(token.pattern.specifier),
token.pattern.minRepresentDigits);
}

if (offset == 0 && zeroOffsetText.has_value()) {
std::memcpy(result, zeroOffsetText->data(), zeroOffsetText->size());
result += zeroOffsetText->size();
break;
}

result += appendTimezoneOffset(offset, result);
if (token.pattern.minRepresentDigits >= 3) {
const auto& piece = timezone->name();
std::memcpy(result, piece.data(), piece.length());
result += piece.length();
break;
}

result += appendTimezoneOffset(
offset, result, token.pattern.minRepresentDigits == 2);
break;
}
case DateTimeFormatSpecifier::WEEK_OF_WEEK_YEAR: {
Expand Down
7 changes: 4 additions & 3 deletions velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
EXPECT_EQ("+05:30", formatDatetime(parseTimestamp("1970-01-01"), "ZZ"));
EXPECT_EQ("+0530", formatDatetime(parseTimestamp("1970-01-01"), "Z"));

// Literal test cases.
EXPECT_EQ("hello", formatDatetime(parseTimestamp("1970-01-01"), "'hello'"));
Expand All @@ -3243,12 +3244,12 @@ TEST_F(DateTimeFunctionsTest, formatDateTime) {
"AD 19 1970 4 Thu 1970 1 1 1 AM 8 8 8 8 3 11 5 Asia/Kolkata",
formatDatetime(
parseTimestamp("1970-01-01 02:33:11.5"),
"G C Y e E y D M d a K h H k m s S zzzz"));
"G C Y e E y D M d a K h H k m s S ZZZ"));
EXPECT_EQ(
"AD 19 1970 4 asdfghjklzxcvbnmqwertyuiop Thu ' 1970 1 1 1 AM 8 8 8 8 3 11 5 1234567890\\\"!@#$%^&*()-+`~{}[];:,./ Asia/Kolkata",
formatDatetime(
parseTimestamp("1970-01-01 02:33:11.5"),
"G C Y e 'asdfghjklzxcvbnmqwertyuiop' E '' y D M d a K h H k m s S 1234567890\\\"!@#$%^&*()-+`~{}[];:,./ zzzz"));
"G C Y e 'asdfghjklzxcvbnmqwertyuiop' E '' y D M d a K h H k m s S 1234567890\\\"!@#$%^&*()-+`~{}[];:,./ ZZZ"));

disableAdjustTimestampToTimezone();
EXPECT_EQ(
Expand Down
2 changes: 1 addition & 1 deletion velox/functions/prestosql/types/TimestampWithTimeZoneType.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ void castToString(
const auto* timestamps = input.as<SimpleVector<int64_t>>();

auto expectedFormatter =
functions::buildJodaDateTimeFormatter("yyyy-MM-dd HH:mm:ss.SSS zzzz");
functions::buildJodaDateTimeFormatter("yyyy-MM-dd HH:mm:ss.SSS ZZZ");
VELOX_CHECK(
!expectedFormatter.hasError(),
"Default format should always be valid, error: {}",
Expand Down

0 comments on commit c89af93

Please sign in to comment.