github-actions[bot] commented on code in PR #68979: URL: https://github.com/apache/doris/pull/68979#discussion_r4256427397
########## 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.HashMap; +import java.util.Locale; +import java.util.Map; +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 HashMap<>(); + 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); + if (name != null) { + return name; + } + String region = findRegion(value); + if (region != null) { + return region; + } + String upper = value.toUpperCase(Locale.ROOT); + // Java's standard UTC names are not present in SHORT_IDS. + if (upper.equals("UT") || upper.equals("Z")) { + return "UTC"; + } + boolean prefixed = upper.startsWith("UTC") || upper.startsWith("GMT"); + String offset = prefixed ? value.substring(3) : value; + Matcher matcher = (usage == Usage.SESSION || prefixed ? OFFSET : TEXT_OFFSET).matcher(offset); + if (!matcher.matches() + || (usage == Usage.SESSION && !prefixed && matcher.group(3) == null)) { + throw new DateTimeException("Invalid timezone: " + value); + } + boolean negative = "-".equals(matcher.group(1)); + int hour = Integer.parseInt(matcher.group(2)); + int minute = matcher.group(3) == null ? 0 : Integer.parseInt(matcher.group(3)); + int second = !prefixed && usage != Usage.SESSION && matcher.group(4) != null + ? Integer.parseInt(matcher.group(4)) : 0; + // Prefixed offsets have the same contract as session offsets in BE. + Usage policy = prefixed ? Usage.SESSION : usage; + int limit = policy == Usage.SESSION ? (negative ? 12 : 14) + : policy == Usage.DATETIME ? 14 : 18; + if (hour > limit || minute >= 60 || second >= 60 + || (policy != Usage.TIMESTAMPTZ && hour == limit && minute != 0) + || (policy == Usage.TIMESTAMPTZ && hour == limit && (minute != 0 || second != 0)) + || (policy == Usage.DATETIME + && (matcher.group(4) != null || (minute != 0 && minute != 30 && minute != 45)))) { + throw new DateTimeException("Invalid timezone offset: " + value); + } + String normalized = String.format(Locale.ROOT, "%s%02d:%02d", negative ? "-" : "+", hour, minute); + if (second != 0) { + normalized += String.format(Locale.ROOT, ":%02d", second); + } + return prefixed ? upper.substring(0, 3) + normalized : normalized; + } + + private static String findAlias(String value) { + for (String alias : TimeUtils.timeZoneAliasMap.keySet()) { + if (alias.equalsIgnoreCase(value)) { + return TimeUtils.getCanonicalTimeZone(alias); + } + } + return null; + } + + private static String findRegion(String value) { + return REGION_NAMES.get(value); Review Comment: [P2] Preserve case-insensitive region lookup for literal evaluation. REGION_NAMES now has exact-case HashMap keys, so this rejects `asia/shanghai` before the existing case-insensitive formatter can canonicalize it. With `time_zone='+00:00'` and non-strict casts, `CAST('2024-01-02 12:00:00 asia/shanghai' AS DATETIME(6))` folds to NULL, while casting the same stored STRING or disabling folding still resolves Shanghai and returns 04:00 in BE (`find_cctz_time_zone` lowercases the key). This worked in FE before the change and also affects the shared literal/CONVERT_TZ parser. Preserve case-insensitive region resolution for these consumers and cover folded versus stored values. This is distinct from the existing Java-only alias thread: it newly rejects a supported region rather than accepting an unsupported alias. ########## be/src/util/timezone_utils.cpp: ########## @@ -237,7 +255,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: [P1] Handle persisted session offsets before tightening the BE resolver. The previous FE accepts `SET GLOBAL time_zone='GMT-12:30'`; session variables restore the String directly from the image/journal, and `CoordinatorContext` still sends its canonical `GMT-12:30` without invoking the new SESSION validator. After an upgrade this resolver returns false, but `RuntimeState::init` ignores that result, leaving its freshly constructed timezone object in UTC ([CCTZ default](https://raw.githubusercontent.com/google/cctz/v2.5/include/cctz/time_zone.h)). Consequently `FROM_UNIXTIME(ts)` for a stored `ts=1704153600` changes from `2024-01-01 11:30:00` to `2024-01-02 00:00:00` silently; an old-FE/new-BE rolling upgrade has the same problem. Preserve decoding of previously valid transmitted offsets or explicitly migrate/reject restored values before execution, and propagate resolver failures instead of proceeding in UTC. This is separate from the existing DATE literal-suffix thread. ########## fe/fe-core/src/test/java/org/apache/doris/common/util/TimeZoneParserTest.java: ########## @@ -0,0 +1,117 @@ +// 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 org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.nereids.trees.expressions.literal.DateTimeV2Literal; +import org.apache.doris.nereids.trees.expressions.literal.TimestampTzLiteral; +import org.apache.doris.nereids.trees.expressions.literal.format.DateTimeChecker; +import org.apache.doris.nereids.types.TimeStampTzType; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.time.DateTimeException; + +class TimeZoneParserTest { + @Test + void sessionBoundaries() { + for (String prefix : new String[] {"", "UTC", "GMT", "utc", "gmt"}) { + for (String offset : new String[] {"+14:01", "+14:30", "+14:45", "-12:01", "-12:30", + "-12:45", "+15:00", "-13:00", "+08:60"}) { + Assertions.assertThrows(DateTimeException.class, + () -> TimeZoneParser.parse(prefix + offset, TimeZoneParser.Usage.SESSION), prefix + offset); + } + for (String offset : new String[] {"+14:00", "-12:00", "+08:01", "+08:15", "-00:30"}) { + Assertions.assertNotNull(TimeZoneParser.parse(prefix + offset, TimeZoneParser.Usage.SESSION)); + } + } + } + + @Test + void offsetSyntaxCannotBypassValidation() { + for (TimeZoneParser.Usage usage : TimeZoneParser.Usage.values()) { + for (String value : new String[] {"UTC8", "GMT8:30", "UTC0", "GMT0:00"}) { + Assertions.assertThrows(DateTimeException.class, + () -> TimeZoneParser.normalize(value, usage), usage + ": " + value); + } + } + for (String value : new String[] {"+00:00:00", "-00:00:00", "+0000", "+00"}) { + Assertions.assertThrows(DateTimeException.class, + () -> TimeZoneParser.normalize(value, TimeZoneParser.Usage.SESSION), value); + } + Assertions.assertThrows(DateTimeException.class, + () -> TimeZoneParser.normalize("+00:00:00", TimeZoneParser.Usage.DATETIME)); + Assertions.assertEquals("+00:00", + TimeZoneParser.normalize("+00:00:00", TimeZoneParser.Usage.TIMESTAMPTZ)); + for (String value : new String[] {"UT", "ut", "Z", "z"}) { + Assertions.assertEquals("UTC", TimeZoneParser.normalize(value, TimeZoneParser.Usage.DATETIME)); + } + } + + @Test + void namesAndSyntax() throws Exception { + Assertions.assertEquals("UTC", TimeUtils.checkTimeZoneValidAndStandardize("utc")); + Assertions.assertThrows(DateTimeException.class, Review Comment: [P2] Expect the public validator's DdlException in this assertion. `checkTimeZoneValidAndStandardize` catches the parser's DateTimeException and calls `ErrorReport.reportDdlException`, which throws DdlException. Therefore this new assertion fails whenever `namesAndSyntax` runs, even though the validator rejects the input as intended. Assert DdlException here (or call TimeZoneParser directly if the test is meant to exercise its exception contract). ########## regression-test/suites/datatype_p0/datetimev2/test_timezone_parser_contract.groovy: ########## @@ -0,0 +1,129 @@ +// 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. + +suite("test_timezone_parser_contract") { + sql "DROP TABLE IF EXISTS timezone_parser_contract_inputs" + sql """CREATE TABLE timezone_parser_contract_inputs (id INT, v STRING) + DUPLICATE KEY(id) DISTRIBUTED BY HASH(id) BUCKETS 1 + PROPERTIES ('replication_num'='1')""" + + def suffixes = ["+00:00", "-00:30", "+14:00", "-12:00", "-13:00", "-14:00", + "+8", "+08", "+0800", "+8:30", "+08:45", "+14:30", "-14:45", + "+08:01", "+08:15", "+08:60", "+24:00", "UTC", "utc", "GMT", "Z", + "zulu", "CST", "cst", "Asia/Shanghai", "asia/shanghai", "Etc/GMT+3", + "America/Port-au-Prince", "America/Does_not_exist", "UTC+08:01", + "GMT-13:00", "UTC+8", "+08:05:43", "-14:21", "+08:00:00", "+18:00", "+18:01", + "+19:00", "GMT0", "EST5EDT", "W-SU", + "+00:00:00", "-00:00:00", "UTC8", "GMT8:30", "UTC0", "GMT0:00"] + def inputs = suffixes.collect { "2024-01-02 12:00:00 ${it}" } + inputs.addAll(["1900-01-01 08:05:43 CST", "1900-01-01 08:05:43 Asia/Shanghai", + "1900-01-01 08:05:43+08:05:43", "20240102120000+0800", "20240102T120000+8:30", + "20240102120000UTC", "20240102T120000UTC"]) + inputs.add(null) + sql "INSERT INTO timezone_parser_contract_inputs VALUES " + inputs.withIndex().collect { v, id -> + "(${id}, ${v == null ? 'NULL' : "'${v}'"})" + }.join(",") + sql "SET enable_sql_cache=false" + sql "SET time_zone='+00:00'" + sql "SET enable_strict_cast=false" + + // Literal folding requests, folding disabled, stored slots and Lambda inputs. + [[false, false], [false, true], [true, false]].eachWithIndex { settings, path -> + sql "SET enable_fold_constant_by_be=${settings[0]}" + sql "SET debug_skip_fold_constant=${settings[1]}" + ["DATETIME(6)", "TIMESTAMPTZ(6)"].eachWithIndex { type, typeId -> + inputs.eachWithIndex { value, id -> + def literal = value == null ? "NULL" : "'${value}'" + "qt_literal_${path}_${typeId}_${id}"("SELECT CAST(${literal} AS ${type})") Review Comment: [P2] Include the generated output file for the new qt suite. There is no `regression-test/data/datatype_p0/datetimev2/test_timezone_parser_contract.out` in this checkout or the supplied changed-file list. Ordinary regression replay reaches the first `qt_literal_*` here and `Suite.quickRunTest` throws `Missing outputFile` rather than checking any results; local generation does not make its output available to CI. Generate the golden through the regression runner, verify the expected timezone results, and commit it with this suite. ########## regression-test/suites/datatype_p0/datetimev2/test_timezone_parser_contract.groovy: ########## @@ -0,0 +1,129 @@ +// 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. + +suite("test_timezone_parser_contract") { + sql "DROP TABLE IF EXISTS timezone_parser_contract_inputs" + sql """CREATE TABLE timezone_parser_contract_inputs (id INT, v STRING) + DUPLICATE KEY(id) DISTRIBUTED BY HASH(id) BUCKETS 1 + PROPERTIES ('replication_num'='1')""" + + def suffixes = ["+00:00", "-00:30", "+14:00", "-12:00", "-13:00", "-14:00", + "+8", "+08", "+0800", "+8:30", "+08:45", "+14:30", "-14:45", + "+08:01", "+08:15", "+08:60", "+24:00", "UTC", "utc", "GMT", "Z", + "zulu", "CST", "cst", "Asia/Shanghai", "asia/shanghai", "Etc/GMT+3", + "America/Port-au-Prince", "America/Does_not_exist", "UTC+08:01", + "GMT-13:00", "UTC+8", "+08:05:43", "-14:21", "+08:00:00", "+18:00", "+18:01", + "+19:00", "GMT0", "EST5EDT", "W-SU", + "+00:00:00", "-00:00:00", "UTC8", "GMT8:30", "UTC0", "GMT0:00"] + def inputs = suffixes.collect { "2024-01-02 12:00:00 ${it}" } + inputs.addAll(["1900-01-01 08:05:43 CST", "1900-01-01 08:05:43 Asia/Shanghai", + "1900-01-01 08:05:43+08:05:43", "20240102120000+0800", "20240102T120000+8:30", + "20240102120000UTC", "20240102T120000UTC"]) + inputs.add(null) + sql "INSERT INTO timezone_parser_contract_inputs VALUES " + inputs.withIndex().collect { v, id -> + "(${id}, ${v == null ? 'NULL' : "'${v}'"})" + }.join(",") + sql "SET enable_sql_cache=false" + sql "SET time_zone='+00:00'" + sql "SET enable_strict_cast=false" + + // Literal folding requests, folding disabled, stored slots and Lambda inputs. + [[false, false], [false, true], [true, false]].eachWithIndex { settings, path -> + sql "SET enable_fold_constant_by_be=${settings[0]}" + sql "SET debug_skip_fold_constant=${settings[1]}" + ["DATETIME(6)", "TIMESTAMPTZ(6)"].eachWithIndex { type, typeId -> + inputs.eachWithIndex { value, id -> + def literal = value == null ? "NULL" : "'${value}'" + "qt_literal_${path}_${typeId}_${id}"("SELECT CAST(${literal} AS ${type})") + } + } + } + sql "SET enable_fold_constant_by_be=false" + sql "SET debug_skip_fold_constant=true" + ["DATETIME(6)", "TIMESTAMPTZ(6)"].eachWithIndex { type, typeId -> + "qt_slots_${typeId}"("SELECT id, CAST(v AS ${type}) FROM timezone_parser_contract_inputs ORDER BY id") + "qt_lambda_${typeId}"("SELECT id, array_map(x -> CAST(x AS ${type}), ARRAY(v)) FROM timezone_parser_contract_inputs ORDER BY id") + } + sql "SET time_zone='+08:00'" + [false, true].eachWithIndex { skip, path -> + sql "SET debug_skip_fold_constant=${skip}" + ["+0800", "America/Port-au-Prince", "Etc/GMT+3"].eachWithIndex { suffix, id -> + "qt_session_literal_${path}_${id}"("SELECT CAST('2024-01-02 12:00:00 ${suffix}' AS TIMESTAMPTZ(6))") + } + } + sql "SET debug_skip_fold_constant=true" + qt_session_slots "SELECT id, CAST(v AS TIMESTAMPTZ(6)) FROM timezone_parser_contract_inputs ORDER BY id" + + ["utc", "Z", "zulu", "cst", "Asia/Shanghai", "UTC+8", "GMT-6", "+14:00", "-12:00"].eachWithIndex { zone, id -> Review Comment: [P2] Use an accepted session-zone spelling in the successful matrix. The third iteration runs `SET time_zone='zulu'`, but the new SESSION parser has no zulu alias and its exact-case region table contains `Zulu`. The input falls through to the offset regex and TimeUtils raises DdlException, so the suite stops before `qt_session_zone_2` and the later CONVERT_TZ/strict cases. This also prevents output generation even after the missing golden is addressed. Use an accepted spelling, or explicitly support lowercase zulu in SESSION validation, and verify the entire suite completes. -- 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]
