Gabriel39 commented on code in PR #68301:
URL: https://github.com/apache/doris/pull/68301#discussion_r4060629965
##########
be/src/exprs/function/cast/cast_to_datetimev2_impl.hpp:
##########
@@ -713,16 +714,39 @@ inline bool
CastToDatetimeV2::from_string_strict_mode_internal(
// minute
SET_PARAMS_RET_FALSE_IFN((consume_digit<UInt32, 2>(ptr, end,
part[1])),
"invalid minute offset '{}'",
std::string {ptr, end});
- SET_PARAMS_RET_FALSE_IFN((part[1] == 0 || part[1] == 30 ||
part[1] == 45),
- "invalid minute offset '{}'",
part[1]);
+ if constexpr (type == DataTimeCastEnumType::TIMESTAMP_TZ) {
+ // TIMESTAMPTZ output preserves historical offsets,
including seconds and
+ // non-quarter-hour minutes. Keep the legacy DATETIME
parser unchanged.
+ SET_PARAMS_RET_FALSE_IFN(part[1] < 60, "invalid minute
offset '{}'", part[1]);
+ if (ptr < end && *ptr == ':') {
+ ++ptr;
+ SET_PARAMS_RET_FALSE_IFN(
+ (consume_digit<UInt32, 2>(ptr, end,
second_offset)),
+ "invalid second offset '{}'", std::string
{ptr, end});
+ SET_PARAMS_RET_FALSE_IFN(second_offset < 60, "invalid
second offset '{}'",
+ second_offset);
+ }
+ } else {
+ SET_PARAMS_RET_FALSE_IFN((part[1] == 0 || part[1] == 30 ||
part[1] == 45),
+ "invalid minute offset '{}'",
part[1]);
+ }
}
- 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),
+ SET_PARAMS_RET_FALSE_IFN(part[0] != 14 || (part[1] == 0 &&
second_offset == 0),
"invalid timezone offset '{}'",
combine_tz_offset(sign, part[0],
part[1]));
+
+ if (second_offset != 0) {
Review Comment:
Fixed in 0765475366abe60678176acbbb82ef7d34366bec. Both strict and fallback
TIMESTAMPTZ parsers now interpret historical wire offsets independently of the
narrower session fixed-zone policy. Tests cover Manila (-15:56:08), Guam
(-14:21), positive offsets beyond +14:00, and malformed offsets. DATE/DATETIME
parsing is retained; this completes the exact-offset round-trip contract
introduced by this PR.
##########
regression-test/suites/datatype_p0/timestamptz/test_timestamptz_historical_offset.groovy:
##########
@@ -0,0 +1,53 @@
+// 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_timestamptz_historical_offset") {
+ def originalZone = sql("select @@time_zone")[0][0]
+ def originalStrict = sql("select @@enable_strict_cast")[0][0]
+ def cases = [
+ ["Asia/Shanghai", "1890-01-01 00:00:00.123456+00:00", "1890-01-01
08:05:43.123456+08:05:43"],
+ ["America/New_York", "1880-01-01 00:00:00.123456+00:00", "1879-12-31
19:03:58.123456-04:56:02"],
+ ["Asia/Shanghai", "2024-01-01 00:00:00.123456+00:00", "2024-01-01
08:00:00.123456+08:00"],
+ ["America/New_York", "2024-01-01 00:00:00.123456+00:00", "2023-12-31
19:00:00.123456-05:00"],
+ ["Asia/Kathmandu", "2024-01-01 00:00:00.123456+00:00", "2024-01-01
05:45:00.123456+05:45"]
+ ]
+ try {
+ for (def testCase : cases) {
+ sql "set time_zone = '${testCase[0]}'"
+ for (def strict : [false, true]) {
+ sql "set enable_strict_cast = ${strict}"
+ // A nonconstant input exercises BE protocol formatting and
parsing instead of
+ // FE constant folding. The offset must retain the instant
when sent back by a client.
+ def wire = sql("""
+ select cast(concat('${testCase[1]}', substring(cast(number
as string), 2))
+ as timestamptz(6))
+ from numbers('number' = '1')
+ """)[0][0].toString()
+ assertEquals(testCase[2], wire)
Review Comment:
These suites already assert their results. Converting all three suites to
golden-output tests is a test-style change, not a newly introduced correctness
or stability fix, so they are not rewritten in this follow-up.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/ArrayFunctionUtils.java:
##########
@@ -0,0 +1,42 @@
+// 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.nereids.trees.expressions.functions.scalar;
+
+import org.apache.doris.nereids.exceptions.AnalysisException;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.types.ArrayType;
+import org.apache.doris.nereids.types.DataType;
+
+/** Argument validation shared by array functions. */
+final class ArrayFunctionUtils {
+ private ArrayFunctionUtils() {
+ }
+
+ static void checkNoVarBinaryArguments(ScalarFunction function) {
Review Comment:
These operations were already unsupported in BE. Moving their rejection into
FE would broaden the function-validation work and is not needed to fix a newly
introduced execution defect.
##########
be/src/exprs/aggregate/aggregate_function_min_max_impl.h:
##########
@@ -141,6 +141,10 @@ AggregateFunctionPtr
create_aggregate_function_single_value(const String& name,
return creator_without_type::create_unary_arguments<
AggregateFunctionsSingleValue<Data<SingleValueDataComplexType>>>(
argument_types, result_is_nullable, attr);
+ case PrimitiveType::TYPE_VARBINARY:
Review Comment:
These aggregate inputs were already unsupported; the new BE branch reports
that unsupported case explicitly. A broader FE rejection inventory is outside
this follow-up.
##########
be/src/exprs/function/in.h:
##########
@@ -105,6 +105,10 @@ class FunctionIn : public IFunction {
if (scope == FunctionContext::THREAD_LOCAL) {
return Status::OK();
}
+ // Binary IO must not route IN through the shared string/storage
predicate implementation.
+ if (context->get_arg_type(0)->get_primitive_type() == TYPE_VARBINARY) {
Review Comment:
VARBINARY IN/NOT IN remains unsupported. Moving the existing
unsupported-type failure to FE is an analysis/diagnostic improvement rather
than a new correctness fix.
--
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]
