Fix inconsistencies between Velox and Presto Java when formatting time zone short/long names (facebookincubator#11447)

Summary:
Pull Request resolved: facebookincubator#11447

While running some more queries in Presto on Velox and Presto on Java, I noticed some cases where
format_datetime would produce in consistent results for the time zone short name ('z', 'zz', or 'zzz')
and the time zone long name ('zzzz' or more).

It seems that Joda does not follow the conventions used by the IANA Time Zone Database or the ICU
library (CLDR), but rather seems to rely on Java's DateFormatSymbols's getZoneStrings method.
This seems to match CLDR except in some cases (which is why I didn't initially notice).

To ensure the behavior of Presto on Velox matches that of the Java implementation, I've written a
small Java program GenTimeZoneNames.java to generate a C++ file TimeZoneNames.cpp with a
mapping from the time zone ID to the abbreviations and long names for that time zone (in standard
time and in daylight savings time).

This diff updates TimeZoneMap's getShortName and getLongName functions to use the mapping
in this generated file to resolve the time zone name. This fixes format_datetime so it's consistent.

This also allowed me to remove the dependency on ICU, so I undid all the changes to build files that
were necessary to make that work.

Reviewed By: kgpai

Differential Revision: D65507863

fbshipit-source-id: f8aa23693705a5ef0630cdd5c6c96950eaa85ecc

Author: Kevin Wilfong

velox/functions/lib/CMakeLists.txt

@@ -24,8 +24,7 @@ velox_link_libraries(velox_functions_util velox_vector velox_common_base)
 velox_add_library(velox_functions_lib_date_time_formatter DateTimeFormatter.cpp
                   DateTimeFormatterBuilder.cpp)
 
-velox_link_libraries(velox_functions_lib_date_time_formatter velox_type_tz
-                     ICU::i18n ICU::uc)
+velox_link_libraries(velox_functions_lib_date_time_formatter velox_type_tz)
 
 velox_add_library(
   velox_functions_lib

velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp

@@ -3566,18 +3566,34 @@ TEST_F(DateTimeFunctionsTest, formatDateTime) {
 
   // Test a long abbreviation.
   setQueryTimeZone("Asia/Colombo");
-  EXPECT_EQ("+0530", formatDatetime(parseTimestamp("1970-10-01"), "z"));
+  EXPECT_EQ("IST", formatDatetime(parseTimestamp("1970-10-01"), "z"));
   EXPECT_EQ(
       "India Standard Time",
       formatDatetime(parseTimestamp("1970-10-01"), "zzzz"));
 
   // Test a long long name.
   setQueryTimeZone("Australia/Eucla");
-  EXPECT_EQ("+0845", formatDatetime(parseTimestamp("1970-10-01"), "z"));
+  EXPECT_EQ("ACWST", formatDatetime(parseTimestamp("1970-10-01"), "z"));
   EXPECT_EQ(
       "Australian Central Western Standard Time",
       formatDatetime(parseTimestamp("1970-10-01"), "zzzz"));
 
+  // Test a time zone that doesn't follow the standard abbrevation in the IANA
+  // Time Zone Database, i.e. it relies on our map in TimeZoneNames.cpp.
+  setQueryTimeZone("Asia/Dubai");
+  // According to the IANA time zone database the abbreviation should be +04.
+  EXPECT_EQ("GST", formatDatetime(parseTimestamp("1970-10-01"), "z"));
+  EXPECT_EQ(
+      "Gulf Standard Time",
+      formatDatetime(parseTimestamp("1970-10-01"), "zzzz"));
+
+  // Test UTC specifically (because it's so common).
+  setQueryTimeZone("UTC");
+  EXPECT_EQ("UTC", formatDatetime(parseTimestamp("1970-10-01"), "z"));
+  EXPECT_EQ(
+      "Coordinated Universal Time",
+      formatDatetime(parseTimestamp("1970-10-01"), "zzzz"));
+
   // Test a time zone name that is linked to another (that gets replaced when
   // converted to a string).
   setQueryTimeZone("US/Pacific");

velox/type/tz/CMakeLists.txt

@@ -19,14 +19,14 @@ velox_add_library(
   TimeZoneMap.h
   TimeZoneDatabase.cpp
   TimeZoneLinks.cpp
-  TimeZoneMap.cpp)
+  TimeZoneMap.cpp
+  TimeZoneNames.h
+  TimeZoneNames.cpp)
 
 velox_link_libraries(
   velox_type_tz
   velox_exception
   velox_external_date
   Boost::regex
   fmt::fmt
-  Folly::folly
-  ICU::i18n
-  ICU::uc)
+  Folly::folly)

velox/type/tz/GenTimeZoneNames.java

@@ -0,0 +1,80 @@
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package velox.type.tz;
+
+import java.text.DateFormatSymbols;
+import java.util.Locale;
+
+public class GenTimeZoneNames {
+  public static void main(String[] args) {
+    System.out.println("/*");
+    System.out.println(" * Copyright (c) Facebook, Inc. and its affiliates.");
+    System.out.println(" *");
+    System.out.println(" * Licensed under the Apache License, Version 2.0 (the \"License\");");
+    System.out.println(" * you may not use this file except in compliance with the License.");
+    System.out.println(" * You may obtain a copy of the License at");
+    System.out.println(" *");
+    System.out.println(" *     http://www.apache.org/licenses/LICENSE-2.0");
+    System.out.println(" *");
+    System.out.println(" * Unless required by applicable law or agreed to in writing, software");
+    System.out.println(" * distributed under the License is distributed on an \"AS IS\" BASIS,");
+    System.out.println(" * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.");
+    System.out.println(" * See the License for the specific language governing permissions and");
+    System.out.println(" * limitations under the License.");
+    System.out.println(" */");
+    System.out.println("");
+    System.out.println("// This file is generated. Do not modify it manually. To re-generate it, run:");
+    System.out.println("//");
+    System.out.println("//  javac velox/type/tz/GenTimeZoneNames.java");
+    System.out.println("//  java velox/type/tz/GenTimeZoneNames > velox/type/tz/TimeZoneNames.cpp");
+    System.out.println("//");
+    System.out.println("// Use the version of Java with which you're running Presto to generate");
+    System.out.println("// compatible names. This version was generated using openjdk version ");
+    System.out.println("// \"1.8.0_362\"");
+    System.out.println("// @generated");
+    System.out.println("");
+    System.out.println("#include \"velox/type/tz/TimeZoneNames.h\"");
+    System.out.println("");
+    System.out.println("#include <folly/Portability.h>");
+    System.out.println("");
+    System.out.println("// This file just contains a single function that returns an, admittedly");
+    System.out.println("// large, hard coded map.");
+    System.out.println("FOLLY_CLANG_DISABLE_WARNING(\"-Wignored-optimization-argument\");");
+    System.out.println("");
+    System.out.println("namespace facebook::velox::tz {");
+    System.out.println("");
+    System.out.println("const std::unordered_map<std::string, TimeZoneNames>& getTimeZoneNames() {");
+    System.out.println("  static auto* timeZoneNames = new std::unordered_map<");
+    System.out.println("      std::string,");
+    System.out.println("      TimeZoneNames>([] {");
+    System.out.println("    // Work around clang compiler bug causing multi-hour compilation");
+    System.out.println("    // with -fsanitize=fuzzer");
+    System.out.println("    // https://github.com/llvm/llvm-project/issues/75666");
+    System.out.println("    return std::unordered_map<std::string, TimeZoneNames>{");
+
+    String[][] zoneStringsEn = DateFormatSymbols.getInstance(Locale.ENGLISH).getZoneStrings();
+    for (String[] zoneStrings : zoneStringsEn) {
+      System.out.println(String.format("        {\"%s\", {\"%s\", \"%s\", \"%s\", \"%s\"}},", zoneStrings[0], zoneStrings[1], zoneStrings[2], zoneStrings[3], zoneStrings[4]));
+    }
+
+    System.out.println("    };");
+    System.out.println("  }());");
+    System.out.println("  return *timeZoneNames;");
+    System.out.println("}");
+    System.out.println("");
+    System.out.println("} // namespace facebook::velox::tz");
+  }
+}

velox/type/tz/TimeZoneMap.cpp

@@ -21,18 +21,10 @@
 #include <folly/container/F14Map.h>
 #include <folly/container/F14Set.h>
 
-#include <unicode/locid.h>
-#include <unicode/timezone.h>
-#include <unicode/unistr.h>
-
-// The ICU libraries define TRUE/FALSE macros which frequently conflict with
-// other libraries that use these as enum/variable names.
-#undef TRUE
-#undef FALSE
-
 #include "velox/common/base/Exceptions.h"
 #include "velox/common/testutil/TestValue.h"
 #include "velox/external/date/tz.h"
+#include "velox/type/tz/TimeZoneNames.h"
 
 using facebook::velox::common::testutil::TestValue;
 
@@ -311,6 +303,43 @@ TDuration toLocalImpl(
   }
   return date::zoned_time{tz, timePoint}.get_local_time().time_since_epoch();
 }
+
+template <bool isLongName>
+std::string getName(
+    TimeZone::milliseconds timestamp,
+    TimeZone::TChoose choose,
+    const date::time_zone* tz,
+    const std::string& timeZoneName) {
+  validateRange(date::sys_time<TimeZone::milliseconds>(timestamp));
+
+  // Time zone offsets only have one name.
+  if (tz == nullptr) {
+    return timeZoneName;
+  }
+
+  static const auto& timeZoneNames = getTimeZoneNames();
+  auto it = timeZoneNames.find(timeZoneName);
+
+  VELOX_CHECK(
+      it != timeZoneNames.end(),
+      "Unable to find short name for time zone: {}",
+      timeZoneName);
+
+  // According to the documentation this is how to determine if DST applies to
+  // a given timestamp in a given time zone.
+  // https://howardhinnant.github.io/date/tz.html#sys_info
+  date::local_time<TimeZone::milliseconds> timePoint{timestamp};
+  bool isDst = getZonedTime(tz, timePoint, choose).get_info().save !=
+      std::chrono::minutes(0);
+
+  if constexpr (isLongName) {
+    return isDst ? it->second.daylightTimeLongName
+                 : it->second.standardTimeLongName;
+  } else {
+    return isDst ? it->second.daylightTimeAbbreviation
+                 : it->second.standardTimeAbbreviation;
+  }
+}
 } // namespace
 
 void validateRange(time_point<std::chrono::seconds> timePoint) {
@@ -419,58 +448,12 @@ TimeZone::milliseconds TimeZone::to_local(
 std::string TimeZone::getShortName(
     TimeZone::milliseconds timestamp,
     TimeZone::TChoose choose) const {
-  date::local_time<milliseconds> timePoint{timestamp};
-  validateRange(date::sys_time<milliseconds>(timestamp));
-
-  // Time zone offsets only have one name (no abbreviations).
-  if (tz_ == nullptr) {
-    return timeZoneName_;
-  }
-
-  return getZonedTime(tz_, timePoint, choose).get_info().abbrev;
+  return getName<false>(timestamp, choose, tz_, timeZoneName_);
 }
 
 std::string TimeZone::getLongName(
     TimeZone::milliseconds timestamp,
     TimeZone::TChoose choose) const {
-  static const icu::Locale locale("en", "US");
-
-  validateRange(date::sys_time<milliseconds>(timestamp));
-
-  // Time zone offsets only have one name.
-  if (tz_ == nullptr) {
-    return timeZoneName_;
-  }
-
-  // Special case for UTC. ICU uses "GMT" for some reason which is an
-  // abbreviation.
-  if (timeZoneID_ == 0) {
-    return "Coordinated Universal Time";
-  }
-
-  // Get the ICU TimeZone by name
-  std::unique_ptr<icu::TimeZone> tz(icu::TimeZone::createTimeZone(
-      icu::UnicodeString(timeZoneName_.data(), timeZoneName_.length())));
-  VELOX_USER_CHECK_NOT_NULL(tz);
-
-  // According to the documentation this is how to determine if DST applies to
-  // a given timestamp in a given time zone.
-  // https://howardhinnant.github.io/date/tz.html#sys_info
-  date::local_time<milliseconds> timePoint{timestamp};
-  bool isDst = getZonedTime(tz_, timePoint, choose).get_info().save !=
-      std::chrono::minutes(0);
-
-  // Construct the long name for the time zone.
-  // Note that ICU does not have DST information for many time zones prior to
-  // 1970, so it's important to specify it explicitly.
-  icu::UnicodeString longName;
-  tz->getDisplayName(
-      isDst, icu::TimeZone::EDisplayType::LONG, locale, longName);
-
-  // Convert the UnicodeString back to a string and write it out
-  std::string longNameStr;
-  longName.toUTF8String(longNameStr);
-
-  return longNameStr;
+  return getName<true>(timestamp, choose, tz_, timeZoneName_);
 }
 } // namespace facebook::velox::tz