github-actions[bot] commented on code in PR #68979:
URL: https://github.com/apache/doris/pull/68979#discussion_r4248955585
##########
fe/fe-core/src/main/java/org/apache/doris/common/util/TimeUtils.java:
##########
@@ -319,42 +317,18 @@ public static long msTimeStringToLong(String timeStr,
TimeZone timeZone) {
// Check if the time zone_value is valid
public static String checkTimeZoneValidAndStandardize(String value) throws
DdlException {
- Function<String, String> standardizeValue = s -> {
- boolean positive = s.charAt(0) != '-';
- String[] parts = s.replaceAll("[+-]", "").split(":");
- return (positive ? "+" : "-") + String.format("%02d:%02d",
- Integer.parseInt(parts[0]), Integer.parseInt(parts[1]));
- };
-
try {
- if (value == null) {
-
ErrorReport.reportDdlException(ErrorCode.ERR_UNKNOWN_TIME_ZONE, "null");
- }
- // match offset type, such as +08:00, -07:00
- Matcher matcher = TIMEZONE_OFFSET_FORMAT_REG.matcher(value);
- // it supports offset and region timezone type, and short zone ids
- boolean match = matcher.matches();
- if (!value.contains("/") && !timeZoneAliasMap.containsKey(value)
&& !match) {
- if ((value.startsWith("GMT") || value.startsWith("UTC"))
- &&
TIMEZONE_OFFSET_FORMAT_REG.matcher(value.substring(3)).matches()) {
- value = value.substring(0, 3) +
standardizeValue.apply(value.substring(3));
- } else {
-
ErrorReport.reportDdlException(ErrorCode.ERR_UNKNOWN_TIME_ZONE, value);
- }
+ String input = value;
+ if (input != null && !input.isEmpty() &&
Character.isDigit(input.charAt(0))) {
+ input = "+" + input;
}
- if (match) {
- value = standardizeValue.apply(value);
-
- // timezone offsets around the world extended from -12:00 to
+14:00
- int tz = Integer.parseInt(value.substring(1, 3)) * 100 +
Integer.parseInt(value.substring(4, 6));
- if (value.charAt(0) == '-' && tz > 1200) {
-
ErrorReport.reportDdlException(ErrorCode.ERR_UNKNOWN_TIME_ZONE, value);
- } else if (value.charAt(0) == '+' && tz > 1400) {
-
ErrorReport.reportDdlException(ErrorCode.ERR_UNKNOWN_TIME_ZONE, value);
+ String normalized = TimeZoneParser.normalize(input,
TimeZoneParser.Usage.SESSION);
Review Comment:
[P2] Preserve normalization in the dynamic-partition ALTER path. This now
accepts asia/shanghai and returns Asia/Shanghai, but
DynamicPartitionUtil.analyzeDynamicPartition discards that return and stores
the original value. On an existing dynamically partitioned table, ALTER TABLE t
SET ('dynamic_partition.time_zone'='asia/shanghai') therefore passes
validation, Env.modifyTableDynamicPartition mutates TableProperty.properties,
then buildDynamicProperty throws when DynamicPartitionProperty calls the
case-sensitive ZoneId.of on the raw value. The statement fails before its
journal entry but leaves the live property map poisoned, so subsequent
dynamic-partition ALTERs fail until the timezone is repaired. Previously this
input failed before mutation. Carry the normalized zone into the analyzed and
logged properties and ensure reconstruction succeeds before changing live
metadata.
##########
fe/fe-core/src/main/java/org/apache/doris/common/util/TimeZoneParser.java:
##########
@@ -0,0 +1,114 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you 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 org.apache.doris.common.util;
+
+import java.time.DateTimeException;
+import java.time.ZoneId;
+import java.util.Collections;
+import java.util.Locale;
+import java.util.Map;
+import java.util.TreeMap;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/** Doris timezone syntax, independent of the Java timezone database's input
grammar. */
+public final class TimeZoneParser {
+ public enum Usage {
+ SESSION, DATETIME, TIMESTAMPTZ
+ }
+
+ private static final Pattern OFFSET =
Pattern.compile("^([+-])([0-9]{1,2})(?::([0-9]{2}))?$");
+ private static final Pattern TEXT_OFFSET = Pattern.compile(
+ "^([+-])([0-9]{1,2})(?::?([0-9]{2}))?(?::([0-9]{2}))?$");
+ private static final Map<String, String> REGION_NAMES;
+
+ static {
+ Map<String, String> names = new
TreeMap<>(String.CASE_INSENSITIVE_ORDER);
+ for (String region : ZoneId.getAvailableZoneIds()) {
+ names.put(region, region);
+ }
+ REGION_NAMES = Collections.unmodifiableMap(names);
+ }
+
+ private TimeZoneParser() {
+ }
+
+ public static String normalize(String value, Usage usage) {
+ if (value == null || value.isEmpty()) {
+ throw new DateTimeException("Empty timezone");
+ }
+ String name = findAlias(value);
Review Comment:
[P2] Keep non-session alias acceptance consistent with BE. The new findAlias
path accepts [Java short
IDs](https://docs.oracle.com/javase/8/docs/api/java/time/ZoneId.html#SHORT_IDS)
such as ACT in literal CAST and CONVERT_TZ, but runtime still passes the
original suffix/argument to TimezoneUtils::find_cctz_time_zone, which has no
ACT alias. With time_zone='+00:00', CAST('2024-01-02 12:00:00 ACT' AS
DATETIME(6)) now folds to 02:30 in FE, while the same value in a stored STRING
slot or with folding disabled returns NULL (or a parse error in strict mode);
CONVERT_TZ(...,'ACT','UTC') likewise succeeds only when folded. Session-zone
canonicalization does not normalize these inputs. Apply equivalent resolution
to the runtime paths or constrain non-session aliases, and cover a Java-only
alias across folded and stored-slot evaluation.
##########
be/src/util/timezone_utils.cpp:
##########
@@ -237,7 +244,8 @@ static bool normalize_offset_string(const std::string&
timezone, bool allow_hour
minute = 0;
}
- if ((!positive && hour > 12) || (positive && hour > 14) || minute >= 60) {
+ const int limit = positive ? 14 : 12;
+ if (hour > limit || (hour == limit && minute != 0) || minute >= 60) {
Review Comment:
[P2] Apply the literal/session split to DATE and legacy casts too. Rejecting
-12:30/-12:45 here also changes CastToDateV2 and CastToDateOrDatetime, whose
strict and non-strict numeric-offset branches still use find_cctz_time_zone.
Those suffixes were valid before this change and still pass FE's DATETIME
literal policy. For example, CAST('2024-01-02 12:00:00-12:30' AS DATE) folds to
2024-01-02 in FE but now returns NULL (or raises a parse error in strict mode)
for a stored STRING value or with folding disabled. DATETIMEV1 is affected as
well. Move these remaining literal consumers onto the policy-neutral
construction path and include DATE/legacy targets in the regression matrix.
##########
be/src/exprs/function/cast/cast_to_datetimev2_impl.hpp:
##########
@@ -733,20 +733,19 @@ inline bool
CastToDatetimeV2::from_string_strict_mode_internal(
"invalid minute offset '{}'",
part[1]);
}
}
- if constexpr (type == DataTimeCastEnumType::TIMESTAMP_TZ) {
- // Wire offsets include historical zones outside the session
fixed-zone range.
- // Use the exact offset even when it has no seconds (for
example, Guam's -14:21).
- const auto offset = static_cast<int>(part[0] * 3600 + part[1]
* 60 + second_offset);
- parsed_tz = cctz::fixed_time_zone(cctz::seconds(sign == '-' ?
-offset : offset));
- } else {
+ if constexpr (type == DataTimeCastEnumType::DATE_TIME) {
SET_PARAMS_RET_FALSE_IFN(part[0] != 14 || part[1] == 0,
"invalid timezone offset '{}'",
combine_tz_offset(sign, part[0],
part[1]));
- SET_PARAMS_RET_FALSE_IFN(
- TimezoneUtils::find_cctz_time_zone(
- combine_tz_offset(sign, part[0], part[1]),
parsed_tz),
- "invalid timezone offset '{}'",
combine_tz_offset(sign, part[0], part[1]));
}
+ if constexpr (type == DataTimeCastEnumType::TIMESTAMP_TZ) {
+ SET_PARAMS_RET_FALSE_IFN(part[0] != 18 || (part[1] == 0 &&
second_offset == 0),
+ "invalid timezone offset '{}'",
+ combine_tz_offset(sign, part[0],
part[1]));
+ }
+ const auto offset = static_cast<int>(part[0] * 3600 + part[1] * 60
+ second_offset);
+ parsed_tz =
+ TimezoneUtils::create_fixed_offset_time_zone(sign == '-' ?
-offset : offset);
Review Comment:
[P2] Preserve cached fixed zones for per-row DATETIME parsing. For common
suffixes such as +08:00, the old DATE_TIME branch copied a preloaded
TimezoneUtils cache entry. This helper calls cctz::fixed_time_zone for every
row instead; in the [pinned CCTZ 2.5
implementation](https://raw.githubusercontent.com/google/cctz/v2.5/src/time_zone_impl.cc)
that goes through LoadTimeZone and acquires the process-wide TimeZoneMutex
even on cache hits. Parallel DATETIME casts and TIMESTAMP_NS parsing now
contend on this global mutex for every nonzero offset, including the non-strict
branch below. Keep the separate literal range validation, but reuse already
constructed zones through a policy-independent immutable cache or equivalent
rather than resolving the fixed zone per row.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
