Merge pull request #50880 from ClickHouse/backport/23.5/50834

Backport #50834 to 23.5: Add compat setting for non-const timezones
ClickHouse · Jun 13, 2023 · 9655251 · 9655251
2 parents dd1ec5b + c28ed3f
commit 9655251
Show file tree

Hide file tree

Showing 25 changed files with 165 additions and 63 deletions.
diff --git a/src/Core/Settings.h b/src/Core/Settings.h
@@ -152,6 +152,7 @@ class IColumn;
     M(Bool, enable_memory_bound_merging_of_aggregation_results, true, "Enable memory bound merging strategy for aggregation.", 0) \
     M(Bool, enable_positional_arguments, true, "Enable positional arguments in ORDER BY, GROUP BY and LIMIT BY", 0) \
     M(Bool, enable_extended_results_for_datetime_functions, false, "Enable date functions like toLastDayOfMonth return Date32 results (instead of Date results) for Date32/DateTime64 arguments.", 0) \
+    M(Bool, allow_nonconst_timezone_arguments, false, "Allow non-const timezone arguments in certain time-related functions like toTimeZone(), fromUnixTimestamp*(), snowflakeToDateTime*()", 0) \
     \
     M(Bool, group_by_use_nulls, false, "Treat columns mentioned in ROLLUP, CUBE or GROUPING SETS as Nullable", 0) \
     \

diff --git a/src/Core/SettingsChangesHistory.h b/src/Core/SettingsChangesHistory.h
@@ -85,6 +85,7 @@ static std::map<ClickHouseVersion, SettingsChangesHistory::SettingsChanges> sett
               {"use_with_fill_by_sorting_prefix", false, true, "Columns preceding WITH FILL columns in ORDER BY clause form sorting prefix. Rows with different values in sorting prefix are filled independently"},
               {"output_format_parquet_compliant_nested_types", false, true, "Change an internal field name in output Parquet file schema."}}},
     {"23.4", {{"allow_suspicious_indices", true, false, "If true, index can defined with identical expressions"},
+              {"allow_nonconst_timezone_arguments", true, false, "Allow non-const timezone arguments in certain time-related functions like toTimeZone(), fromUnixTimestamp*(), snowflakeToDateTime*()."},
               {"connect_timeout_with_failover_ms", 50, 1000, "Increase default connect timeout because of async connect"},
               {"connect_timeout_with_failover_secure_ms", 100, 1000, "Increase default secure connect timeout because of async connect"},
               {"hedged_connection_timeout_ms", 100, 50, "Start new connection in hedged requests after 50 ms instead of 100 to correspond with previous connect timeout"}}},

diff --git a/src/Functions/FunctionDateOrDateTimeAddInterval.h b/src/Functions/FunctionDateOrDateTimeAddInterval.h
@@ -679,7 +679,7 @@ class FunctionDateOrDateTimeAddInterval : public IFunction
         }
         else if constexpr (std::is_same_v<ResultDataType, DataTypeDateTime>)
         {
-            return std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, 2, 0));
+            return std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, 2, 0, false));
         }
         else if constexpr (std::is_same_v<ResultDataType, DataTypeDateTime64>)
         {
@@ -696,7 +696,7 @@ class FunctionDateOrDateTimeAddInterval : public IFunction
                     return {};
                 });
 
-            auto timezone = extractTimeZoneNameFromFunctionArguments(arguments, 2, 0);
+            auto timezone = extractTimeZoneNameFromFunctionArguments(arguments, 2, 0, false);
             if (const auto* datetime64_type = typeid_cast<const DataTypeDateTime64 *>(arguments[0].type.get()))
             {
                 const auto from_scale = datetime64_type->getScale();

diff --git a/src/Functions/FunctionDateOrDateTimeToDateOrDate32.h b/src/Functions/FunctionDateOrDateTimeToDateOrDate32.h
@@ -36,7 +36,7 @@ class FunctionDateOrDateTimeToDateOrDate32 : public IFunctionDateOrDateTime<Tran
         /// If the time zone is specified but empty, throw an exception.
         /// only validate the time_zone part if the number of arguments is 2.
         if ((which.isDateTime() || which.isDateTime64()) && arguments.size() == 2
-            && extractTimeZoneNameFromFunctionArguments(arguments, 1, 0).empty())
+            && extractTimeZoneNameFromFunctionArguments(arguments, 1, 0, false).empty())
             throw Exception(ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT,
                 "Function {} supports a 2nd argument (optional) that must be a valid time zone",
                 this->getName());

diff --git a/src/Functions/FunctionDateOrDateTimeToDateTimeOrDateTime64.h b/src/Functions/FunctionDateOrDateTimeToDateTimeOrDateTime64.h
@@ -34,7 +34,7 @@ class FunctionDateOrDateTimeToDateTimeOrDateTime64 : public IFunctionDateOrDateT
 
         WhichDataType which(from_type);
 
-        std::string time_zone = extractTimeZoneNameFromFunctionArguments(arguments, 1, 0);
+        std::string time_zone = extractTimeZoneNameFromFunctionArguments(arguments, 1, 0, false);
 
         /// If the time zone is specified but empty, throw an exception.
         /// only validate the time_zone part if the number of arguments is 2.

diff --git a/src/Functions/FunctionDateOrDateTimeToSomething.h b/src/Functions/FunctionDateOrDateTimeToSomething.h
@@ -24,7 +24,7 @@ class FunctionDateOrDateTimeToSomething : public IFunctionDateOrDateTime<Transfo
         /// If the time zone is specified but empty, throw an exception.
         if constexpr (std::is_same_v<ToDataType, DataTypeDateTime>)
         {
-            std::string time_zone = extractTimeZoneNameFromFunctionArguments(arguments, 1, 0);
+            std::string time_zone = extractTimeZoneNameFromFunctionArguments(arguments, 1, 0, false);
             /// only validate the time_zone part if the number of arguments is 2. This is mainly
             /// to accommodate functions like toStartOfDay(today()), toStartOfDay(yesterday()) etc.
             if (arguments.size() == 2 && time_zone.empty())
@@ -53,7 +53,7 @@ class FunctionDateOrDateTimeToSomething : public IFunctionDateOrDateTime<Transfo
                 scale = std::max(source_scale, static_cast<Int64>(9));
             }
 
-            return std::make_shared<ToDataType>(scale, extractTimeZoneNameFromFunctionArguments(arguments, 1, 0));
+            return std::make_shared<ToDataType>(scale, extractTimeZoneNameFromFunctionArguments(arguments, 1, 0, false));
         }
         else
             return std::make_shared<ToDataType>();

diff --git a/src/Functions/FunctionSnowflake.h b/src/Functions/FunctionSnowflake.h
@@ -6,6 +6,7 @@
 #include <DataTypes/DataTypeDateTime64.h>
 #include <DataTypes/DataTypesNumber.h>
 #include <Columns/ColumnsNumber.h>
+#include <Interpreters/Context.h>
 
 #include <base/arithmeticOverflow.h>
 
@@ -72,9 +73,13 @@ class FunctionSnowflakeToDateTime : public IFunction
 {
 private:
     const char * name;
+    const bool allow_nonconst_timezone_arguments;
 
 public:
-    explicit FunctionSnowflakeToDateTime(const char * name_) : name(name_) { }
+    explicit FunctionSnowflakeToDateTime(const char * name_, ContextPtr context)
+        : name(name_)
+        , allow_nonconst_timezone_arguments(context->getSettings().allow_nonconst_timezone_arguments)
+    {}
 
     String getName() const override { return name; }
     size_t getNumberOfArguments() const override { return 0; }
@@ -92,7 +97,7 @@ class FunctionSnowflakeToDateTime : public IFunction
 
         std::string timezone;
         if (arguments.size() == 2)
-            timezone = extractTimeZoneNameFromFunctionArguments(arguments, 1, 0);
+            timezone = extractTimeZoneNameFromFunctionArguments(arguments, 1, 0, allow_nonconst_timezone_arguments);
 
         return std::make_shared<DataTypeDateTime>(timezone);
     }
@@ -162,9 +167,13 @@ class FunctionSnowflakeToDateTime64 : public IFunction
 {
 private:
     const char * name;
+    const bool allow_nonconst_timezone_arguments;
 
 public:
-    explicit FunctionSnowflakeToDateTime64(const char * name_) : name(name_) { }
+    explicit FunctionSnowflakeToDateTime64(const char * name_, ContextPtr context)
+        : name(name_)
+        , allow_nonconst_timezone_arguments(context->getSettings().allow_nonconst_timezone_arguments)
+    {}
 
     String getName() const override { return name; }
     size_t getNumberOfArguments() const override { return 0; }
@@ -182,7 +191,7 @@ class FunctionSnowflakeToDateTime64 : public IFunction
 
         std::string timezone;
         if (arguments.size() == 2)
-            timezone = extractTimeZoneNameFromFunctionArguments(arguments, 1, 0);
+            timezone = extractTimeZoneNameFromFunctionArguments(arguments, 1, 0, allow_nonconst_timezone_arguments);
 
         return std::make_shared<DataTypeDateTime64>(3, timezone);
     }

diff --git a/src/Functions/FunctionUnixTimestamp64.h b/src/Functions/FunctionUnixTimestamp64.h
@@ -6,6 +6,7 @@
 #include <DataTypes/DataTypeDateTime64.h>
 #include <DataTypes/DataTypesNumber.h>
 #include <Columns/ColumnsNumber.h>
+#include <Interpreters/Context.h>
 
 #include <base/arithmeticOverflow.h>
 
@@ -99,11 +100,13 @@ class FunctionFromUnixTimestamp64 : public IFunction
 private:
     size_t target_scale;
     const char * name;
+    const bool allow_nonconst_timezone_arguments;
 public:
-    FunctionFromUnixTimestamp64(size_t target_scale_, const char * name_)
-        : target_scale(target_scale_), name(name_)
-    {
-    }
+    FunctionFromUnixTimestamp64(size_t target_scale_, const char * name_, ContextPtr context)
+        : target_scale(target_scale_)
+        , name(name_)
+        , allow_nonconst_timezone_arguments(context->getSettings().allow_nonconst_timezone_arguments)
+    {}
 
     String getName() const override { return name; }
     size_t getNumberOfArguments() const override { return 0; }
@@ -121,7 +124,7 @@ class FunctionFromUnixTimestamp64 : public IFunction
 
         std::string timezone;
         if (arguments.size() == 2)
-            timezone = extractTimeZoneNameFromFunctionArguments(arguments, 1, 0);
+            timezone = extractTimeZoneNameFromFunctionArguments(arguments, 1, 0, allow_nonconst_timezone_arguments);
 
         return std::make_shared<DataTypeDateTime64>(target_scale, timezone);
     }

diff --git a/src/Functions/FunctionsConversion.h b/src/Functions/FunctionsConversion.h
@@ -1796,13 +1796,13 @@ class FunctionConvert : public IFunction
 
                 if (to_datetime64 || scale != 0) /// toDateTime('xxxx-xx-xx xx:xx:xx', 0) return DateTime
                     return std::make_shared<DataTypeDateTime64>(scale,
-                        extractTimeZoneNameFromFunctionArguments(arguments, timezone_arg_position, 0));
+                        extractTimeZoneNameFromFunctionArguments(arguments, timezone_arg_position, 0, false));
 
-                return std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, timezone_arg_position, 0));
+                return std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, timezone_arg_position, 0, false));
             }
 
             if constexpr (std::is_same_v<ToDataType, DataTypeDateTime>)
-                return std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, timezone_arg_position, 0));
+                return std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, timezone_arg_position, 0, false));
             else if constexpr (std::is_same_v<ToDataType, DataTypeDateTime64>)
                 throw Exception(ErrorCodes::LOGICAL_ERROR, "Unexpected branch in code of conversion function: it is a bug.");
             else
@@ -2067,7 +2067,7 @@ class FunctionConvertFromString : public IFunction
             UInt64 scale = to_datetime64 ? DataTypeDateTime64::default_scale : 0;
             if (arguments.size() > 1)
                 scale = extractToDecimalScale(arguments[1]);
-            const auto timezone = extractTimeZoneNameFromFunctionArguments(arguments, 2, 0);
+            const auto timezone = extractTimeZoneNameFromFunctionArguments(arguments, 2, 0, false);
 
             res = scale == 0 ? res = std::make_shared<DataTypeDateTime>(timezone) : std::make_shared<DataTypeDateTime64>(scale, timezone);
         }
@@ -2117,7 +2117,7 @@ class FunctionConvertFromString : public IFunction
             }
 
             if constexpr (std::is_same_v<ToDataType, DataTypeDateTime>)
-                res = std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, 1, 0));
+                res = std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, 1, 0, false));
             else if constexpr (std::is_same_v<ToDataType, DataTypeDateTime64>)
                 throw Exception(ErrorCodes::LOGICAL_ERROR, "LOGICAL ERROR: It is a bug.");
             else if constexpr (to_decimal)

diff --git a/src/Functions/FunctionsTimeWindow.cpp b/src/Functions/FunctionsTimeWindow.cpp
@@ -138,7 +138,7 @@ struct TimeWindowImpl<TUMBLE>
         if (result_type_is_date)
             data_type = std::make_shared<DataTypeDate>();
         else
-            data_type = std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, 2, 0));
+            data_type = std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, 2, 0, false));
 
         return std::make_shared<DataTypeTuple>(DataTypes{data_type, data_type});
     }
@@ -322,7 +322,7 @@ struct TimeWindowImpl<HOP>
         if (result_type_is_date)
             data_type = std::make_shared<DataTypeDate>();
         else
-            data_type = std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, 3, 0));
+            data_type = std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, 3, 0, false));
         return std::make_shared<DataTypeTuple>(DataTypes{data_type, data_type});
     }
 

diff --git a/src/Functions/date_trunc.cpp b/src/Functions/date_trunc.cpp
@@ -107,7 +107,7 @@ class FunctionDateTrunc : public IFunction
         if (result_type_is_date)
             return std::make_shared<DataTypeDate>();
         else
-            return std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, 2, 1));
+            return std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, 2, 1, false));
     }
 
     bool useDefaultImplementationForConstants() const override { return true; }

diff --git a/src/Functions/extractTimeZoneFromFunctionArguments.cpp b/src/Functions/extractTimeZoneFromFunctionArguments.cpp
@@ -30,10 +30,11 @@ std::string extractTimeZoneNameFromColumn(const IColumn * column, const String &
 }
 
 
-std::string extractTimeZoneNameFromFunctionArguments(const ColumnsWithTypeAndName & arguments, size_t time_zone_arg_num, size_t datetime_arg_num)
+std::string extractTimeZoneNameFromFunctionArguments(const ColumnsWithTypeAndName & arguments, size_t time_zone_arg_num, size_t datetime_arg_num, bool allow_nonconst_timezone_arguments)
 {
     /// Explicit time zone may be passed in last argument.
-    if (arguments.size() == time_zone_arg_num + 1)
+    if ((arguments.size() == time_zone_arg_num + 1)
+       && (!allow_nonconst_timezone_arguments || arguments[time_zone_arg_num].column))
     {
         return extractTimeZoneNameFromColumn(arguments[time_zone_arg_num].column.get(), arguments[time_zone_arg_num].name);
     }

diff --git a/src/Functions/extractTimeZoneFromFunctionArguments.h b/src/Functions/extractTimeZoneFromFunctionArguments.h
@@ -16,8 +16,16 @@ std::string extractTimeZoneNameFromColumn(const IColumn * column, const String &
 
 /// Determine working timezone either from optional argument with time zone name or from time zone in DateTime type of argument.
 /// Returns empty string if default time zone should be used.
+///
+/// Parameter allow_nonconst_timezone_arguments toggles if non-const timezone function arguments are accepted (legacy behavior) or not. The
+/// problem with the old behavior is that the timezone is part of the type, and not part of the value. This lead to confusion and unexpected
+/// results.
+/// - For new functions, set allow_nonconst_timezone_arguments = false.
+/// - For existing functions
+///    - which disallow non-const timezone arguments anyways (e.g. getArgumentsThatAreAlwaysConstant()), set allow_nonconst_timezone_arguments = false,
+///    - which allow non-const timezone arguments, set allow_nonconst_timezone_arguments according to the corresponding setting.
 std::string extractTimeZoneNameFromFunctionArguments(
-    const ColumnsWithTypeAndName & arguments, size_t time_zone_arg_num, size_t datetime_arg_num);
+    const ColumnsWithTypeAndName & arguments, size_t time_zone_arg_num, size_t datetime_arg_num, bool allow_nonconst_timezone_arguments);
 
 const DateLUTImpl & extractTimeZoneFromFunctionArguments(
     const ColumnsWithTypeAndName & arguments, size_t time_zone_arg_num, size_t datetime_arg_num);

diff --git a/src/Functions/fromUnixTimestamp64Micro.cpp b/src/Functions/fromUnixTimestamp64Micro.cpp
@@ -7,8 +7,8 @@ namespace DB
 REGISTER_FUNCTION(FromUnixTimestamp64Micro)
 {
     factory.registerFunction("fromUnixTimestamp64Micro",
-        [](ContextPtr){ return std::make_unique<FunctionToOverloadResolverAdaptor>(
-            std::make_shared<FunctionFromUnixTimestamp64>(6, "fromUnixTimestamp64Micro")); });
+        [](ContextPtr context){ return std::make_unique<FunctionToOverloadResolverAdaptor>(
+            std::make_shared<FunctionFromUnixTimestamp64>(6, "fromUnixTimestamp64Micro", context)); });
 }
 
 }
diff --git a/src/Functions/fromUnixTimestamp64Milli.cpp b/src/Functions/fromUnixTimestamp64Milli.cpp
@@ -7,8 +7,8 @@ namespace DB
 REGISTER_FUNCTION(FromUnixTimestamp64Milli)
 {
     factory.registerFunction("fromUnixTimestamp64Milli",
-        [](ContextPtr){ return std::make_unique<FunctionToOverloadResolverAdaptor>(
-            std::make_shared<FunctionFromUnixTimestamp64>(3, "fromUnixTimestamp64Milli")); });
+        [](ContextPtr context){ return std::make_unique<FunctionToOverloadResolverAdaptor>(
+            std::make_shared<FunctionFromUnixTimestamp64>(3, "fromUnixTimestamp64Milli", context)); });
 }
 
 }
diff --git a/src/Functions/fromUnixTimestamp64Nano.cpp b/src/Functions/fromUnixTimestamp64Nano.cpp
@@ -7,8 +7,8 @@ namespace DB
 REGISTER_FUNCTION(FromUnixTimestamp64Nano)
 {
     factory.registerFunction("fromUnixTimestamp64Nano",
-        [](ContextPtr){ return std::make_unique<FunctionToOverloadResolverAdaptor>(
-            std::make_shared<FunctionFromUnixTimestamp64>(9, "fromUnixTimestamp64Nano")); });
+        [](ContextPtr context){ return std::make_unique<FunctionToOverloadResolverAdaptor>(
+            std::make_shared<FunctionFromUnixTimestamp64>(9, "fromUnixTimestamp64Nano", context)); });
 }
 
 }
diff --git a/src/Functions/now.cpp b/src/Functions/now.cpp
@@ -5,6 +5,7 @@
 #include <Functions/FunctionFactory.h>
 #include <Functions/IFunction.h>
 #include <Functions/extractTimeZoneFromFunctionArguments.h>
+#include <Interpreters/Context.h>
 
 namespace DB
 {
@@ -87,7 +88,10 @@ class NowOverloadResolver : public IFunctionOverloadResolver
     bool isVariadic() const override { return true; }
 
     size_t getNumberOfArguments() const override { return 0; }
-    static FunctionOverloadResolverPtr create(ContextPtr) { return std::make_unique<NowOverloadResolver>(); }
+    static FunctionOverloadResolverPtr create(ContextPtr context) { return std::make_unique<NowOverloadResolver>(context); }
+    explicit NowOverloadResolver(ContextPtr context)
+        : allow_nonconst_timezone_arguments(context->getSettings().allow_nonconst_timezone_arguments)
+    {}
 
     DataTypePtr getReturnTypeImpl(const ColumnsWithTypeAndName & arguments) const override
     {
@@ -102,7 +106,7 @@ class NowOverloadResolver : public IFunctionOverloadResolver
         }
         if (arguments.size() == 1)
         {
-            return std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, 0, 0));
+            return std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, 0, 0, allow_nonconst_timezone_arguments));
         }
         return std::make_shared<DataTypeDateTime>();
     }
@@ -121,10 +125,12 @@ class NowOverloadResolver : public IFunctionOverloadResolver
         if (arguments.size() == 1)
             return std::make_unique<FunctionBaseNow>(
                 time(nullptr), DataTypes{arguments.front().type},
-                std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, 0, 0)));
+                std::make_shared<DataTypeDateTime>(extractTimeZoneNameFromFunctionArguments(arguments, 0, 0, allow_nonconst_timezone_arguments)));
 
         return std::make_unique<FunctionBaseNow>(time(nullptr), DataTypes(), std::make_shared<DataTypeDateTime>());
     }
+private:
+    const bool allow_nonconst_timezone_arguments;
 };
 
 }