Validate session time zone in Config (facebookincubator#9666)

Summary:
Validate configurations when they are being set to reject "bad" settings early
on.

Pull Request resolved: facebookincubator#9666

Reviewed By: amitkdutta

Differential Revision: D56766104

Pulled By: kagamiori

fbshipit-source-id: 35d5fd0fed1a808310d4bf5baa95e5b0f4b3327c

velox/core/Config.cpp

@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 #include "velox/core/Config.h"
+#include "velox/core/QueryConfig.h"
+#include "velox/type/tz/TimeZoneMap.h"
 
 namespace facebook::velox::core {
 
@@ -46,4 +48,11 @@ bool MemConfigMutable::isValueExists(const std::string& key) const {
   return lockedValues->find(key) != lockedValues->end();
 }
 
+void MemConfig::validateConfig() {
+  // Validate if timezone name can be recognized.
+  if (isValueExists(QueryConfig::kSessionTimezone)) {
+    util::getTimeZoneID(values_[QueryConfig::kSessionTimezone]);
+  }
+}
+
 } // namespace facebook::velox::core

velox/core/Config.h

@@ -70,12 +70,16 @@ namespace core {
 class MemConfig : public Config {
  public:
   explicit MemConfig(const std::unordered_map<std::string, std::string>& values)
-      : values_(values) {}
+      : values_(values) {
+    validateConfig();
+  }
 
   explicit MemConfig() : values_{} {}
 
   explicit MemConfig(std::unordered_map<std::string, std::string>&& values)
-      : values_(std::move(values)) {}
+      : values_(std::move(values)) {
+    validateConfig();
+  }
 
   folly::Optional<std::string> get(const std::string& key) const override;
 
@@ -90,6 +94,9 @@ class MemConfig : public Config {
   }
 
  private:
+  // Validate if configurations are valid.
+  void validateConfig();
+
   std::unordered_map<std::string, std::string> values_;
 };
 

velox/core/tests/QueryConfigTest.cpp

@@ -46,8 +46,23 @@ TEST_F(QueryConfigTest, setConfig) {
   ASSERT_TRUE(config.isLegacyCast());
 }
 
+TEST_F(QueryConfigTest, invalidConfig) {
+  std::unordered_map<std::string, std::string> configData(
+      {{QueryConfig::kSessionTimezone, "Invalid"}});
+  VELOX_ASSERT_USER_THROW(
+      std::make_shared<QueryCtx>(nullptr, std::move(configData)),
+      "Unknown time zone: 'Invalid'");
+
+  auto queryCtx = std::make_shared<QueryCtx>(nullptr);
+  VELOX_ASSERT_USER_THROW(
+      queryCtx->testingOverrideConfigUnsafe({
+          {core::QueryConfig::kSessionTimezone, ""},
+      }),
+      "Unknown time zone: ''");
+}
+
 TEST_F(QueryConfigTest, memConfig) {
-  const std::string tz = "timezone1";
+  const std::string tz = "UTC";
   const std::unordered_map<std::string, std::string> configData(
       {{QueryConfig::kSessionTimezone, tz}});
 
@@ -72,7 +87,7 @@ TEST_F(QueryConfigTest, memConfig) {
         tz,
         cfg.Config::get<std::string>(QueryConfig::kSessionTimezone).value());
     ASSERT_FALSE(cfg.Config::get<std::string>("missing-entry").has_value());
-    const std::string tz2 = "timezone2";
+    const std::string tz2 = "PST";
     ASSERT_NO_THROW(cfg.setValue(QueryConfig::kSessionTimezone, tz2));
     ASSERT_EQ(
         tz2,

velox/expression/tests/CastExprTest.cpp

@@ -710,7 +710,6 @@ TEST_F(CastExprTest, dateToTimestamp) {
 }
 
 TEST_F(CastExprTest, timestampToDate) {
-  setTimezone("");
   std::vector<std::optional<Timestamp>> inputTimestamps = {
       Timestamp(0, 0),
       Timestamp(946684800, 0),
@@ -766,6 +765,10 @@ TEST_F(CastExprTest, timestampInvalid) {
 }
 
 TEST_F(CastExprTest, timestampAdjustToTimezone) {
+  // Empty timezone is assumed to be GMT.
+  testCast<std::string, Timestamp>(
+      "timestamp", {"1970-01-01"}, {Timestamp(0, 0)});
+
   setTimezone("America/Los_Angeles");
 
   // Expect unix epochs to be converted to LA timezone (8h offset).
@@ -789,21 +792,10 @@ TEST_F(CastExprTest, timestampAdjustToTimezone) {
           std::nullopt,
           Timestamp(957164400, 0),
       });
-
-  // Empty timezone is assumed to be GMT.
-  setTimezone("");
-  testCast<std::string, Timestamp>(
-      "timestamp", {"1970-01-01"}, {Timestamp(0, 0)});
 }
 
 TEST_F(CastExprTest, timestampAdjustToTimezoneInvalid) {
-  auto testFunc = [&]() {
-    testCast<std::string, Timestamp>(
-        "timestamp", {"1970-01-01"}, {Timestamp(1, 0)});
-  };
-
-  setTimezone("bla");
-  EXPECT_THROW(testFunc(), std::runtime_error);
+  VELOX_ASSERT_USER_THROW(setTimezone("bla"), "Unknown time zone: 'bla'");
 }
 
 TEST_F(CastExprTest, date) {

velox/functions/sparksql/tests/MakeTimestampTest.cpp

@@ -130,6 +130,11 @@ TEST_F(MakeTimestampTest, errors) {
         {INTEGER(), INTEGER(), INTEGER(), INTEGER(), INTEGER(), microsType});
   };
 
+  // Throw if no session time zone.
+  VELOX_ASSERT_USER_THROW(
+      testInvalidArguments(60007000, DECIMAL(16, 6)),
+      "make_timestamp requires session time zone to be set.");
+
   setQueryTimeZone("Asia/Shanghai");
   // Invalid input returns null.
   const auto year = makeFlatVector<int32_t>(
@@ -166,11 +171,6 @@ TEST_F(MakeTimestampTest, errors) {
       "Scalar function signature is not supported: "
       "make_timestamp(INTEGER, INTEGER, INTEGER, INTEGER, INTEGER, "
       "DECIMAL(16, 8)).");
-  // Throw if no session time zone.
-  setQueryTimeZone("");
-  VELOX_ASSERT_THROW(
-      testInvalidArguments(60007000, DECIMAL(16, 6)),
-      "make_timestamp requires session time zone to be set.");
 }
 
 TEST_F(MakeTimestampTest, invalidTimezone) {
@@ -184,13 +184,7 @@ TEST_F(MakeTimestampTest, invalidTimezone) {
       {45678000, 1e6, 6e7, 59999999, std::nullopt}, microsType);
   auto data = makeRowVector({year, month, day, hour, minute, micros});
 
-  // Invalid time zone from query config.
-  setQueryTimeZone("Invalid");
-  VELOX_ASSERT_USER_THROW(
-      evaluate("make_timestamp(c0, c1, c2, c3, c4, c5)", data),
-      "Unknown time zone: 'Invalid'");
-
-  setQueryTimeZone("");
+  // Time zone is not set.
   VELOX_ASSERT_USER_THROW(
       evaluate("make_timestamp(c0, c1, c2, c3, c4, c5)", data),
       "make_timestamp requires session time zone to be set.");