github-actions[bot] commented on code in PR #68301:
URL: https://github.com/apache/doris/pull/68301#discussion_r4059258454
##########
be/src/core/value/timestamptz_value.cpp:
##########
@@ -38,6 +39,13 @@ std::string TimestampTzValue::to_string(const
cctz::time_zone& tz, int scale) co
auto lookup_result = tz.lookup(cur_tz_time);
cctz::civil_second civ = lookup_result.cs;
+ // UTC storage bounds do not guarantee a representable session-local year.
Reject
+ // overflow before DateTimeV2 formatting could produce an offset-only wire
value.
+ if (civ.year() < 0 || civ.year() > 9999) {
Review Comment:
[P2] Keep the remaining TIMESTAMP_NS error paths from re-entering this
throwing formatter. With the session zone +08:00, the maximum UTC TIMESTAMPTZ
first fails local conversion because the year becomes 10000; strict
TIMESTAMPTZ-to-TIMESTAMP_NS cast and TIMESTAMPTZ/TIMESTAMP_NS comparison then
call to_string(local_zone) while building their InvalidArgument Status, so this
exception escapes before that Status is returned. The parallel cast paths
changed in this PR already render utc_dt() instead. Please update these two
missed callers the same way and add strict-cast and comparison boundary tests.
##########
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:
[P2] Accept every historical offset the formatter emits.
TimestampTzValue::to_string() now emits the exact cctz offset, but both parser
branches still cap the hour at 14 and this seconds branch additionally caps
negative offsets at 12. IANA history includes larger values (for example
Asia/Manila used -15:56:08 before 1845), so Doris can return a TIMESTAMPTZ
string that it cannot parse back. Please align both parser branches with the
formatter's range and add strict/non-strict round-trip coverage for an extreme
historical zone.
##########
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:
[P2] Record these deterministic results as golden output. All three new
TIMESTAMPTZ suites assert successful query results directly and add no .out
files, while this repository requires determined regression results to use
qt_/order_qt_ cases with runner-generated output. Please keep the test { sql;
exception } blocks for failures, convert the successful cases in all three
suites, and generate their checked-in .out files.
##########
be/src/exec/sink/writer/iceberg/partition_transformers.cpp:
##########
@@ -46,6 +46,12 @@ const std::chrono::sys_days
PartitionColumnTransformUtils::EPOCH = std::chrono::
std::unique_ptr<PartitionColumnTransform> PartitionColumnTransforms::create(
const doris::iceberg::PartitionField& field, const DataTypePtr&
source_type) {
auto& transform = field.transform();
+ // Identity/void only carry values; computed binary partition transforms
are unsupported.
+ if (source_type->get_primitive_type() == TYPE_VARBINARY && transform !=
"identity" &&
Review Comment:
[P1] Reject identity until the writer supports binary partitions. This
branch deliberately leaves identity enabled, but
IdentityPartitionColumnTransform returns ColumnVarbinary and
VIcebergTableWriter::_get_iceberg_partition_value has no TYPE_VARBINARY arm, so
a non-null dynamic identity partition falls into "Unsupported type for
partition". The later partition-string and FE commit reconstruction paths also
have no binary-safe representation. With enable.mapping.varbinary=true, inserts
into Iceberg BINARY/FIXED identity partitions therefore fail. Please either
reject identity here too, or implement byte-safe
extraction/transport/reconstruction and add an end-to-end identity test.
##########
be/src/exec/common/hash_table/hash_key_type.h:
##########
@@ -102,6 +102,13 @@ inline HashKeyType get_hash_key_type_fixed(const
std::vector<DataTypePtr>& data_
}
inline HashKeyType get_hash_key_type(const std::vector<DataTypePtr>&
data_types) {
+ // Reject binary before the multi-key serialization fallback can enable
joins or grouping.
+ for (const auto& type : data_types) {
+ if (type->get_primitive_type() == TYPE_VARBINARY) {
Review Comment:
[P1] Preserve the existing serialized path for supported multi-key
consumers. This pre-check now runs before the multi-key branch, so it also
rejects keys that previously selected HashKeyType::serialized: default-enabled
PartitionTopN windows, multi-column INTERSECT/EXCEPT, and recursive UNION
DISTINCT. Their PartitionedHashMapVariants, SetDataVariants, and
DistinctDataVariants all implement serialized keys, and ColumnVarbinary
supplies serialization; only single-key VARBINARY was already unsupported.
Please move the rejection to the join/group consumers that require it (or
otherwise retain these byte-safe consumers) and add multi-key window,
set-operation, and recursive-CTE coverage.
--
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]
