Skip to content

Commit 0765475

Browse files
Gabriel39Gabriel39
committed
[fix](types) Close timestamp wire parsing and error-reporting gaps
### What problem does this PR solve? Historical TIMESTAMPTZ output could contain offsets rejected by its parsers. Parse wire offsets independently of session fixed-zone limits in both modes. The new local-year formatter exception also interrupted TIMESTAMP_NS cast and comparison error reporting; render UTC values when constructing those errors. ### Release note Preserve historical TIMESTAMPTZ round trips and timestamp conversion errors. ### Check List (For Author) - Test: 31 focused ASAN BE tests passed, including ordinary date/datetime parsing; three targeted tests failed before the fixes. clang-format 16 and build hygiene passed. Full clang-tidy remains affected by pre-existing diagnostics. - Behavior changed: Yes; accept historical wire offsets and preserve InvalidArgument/NULL handling. - Does this need documentation: No.
1 parent f68c0ba commit 0765475

5 files changed

Lines changed: 67 additions & 24 deletions

File tree

‎be/src/exprs/function/cast/cast_to_datetimev2_impl.hpp‎

Lines changed: 17 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -706,7 +706,9 @@ inline bool CastToDatetimeV2::from_string_strict_mode_internal(
706706
SET_PARAMS_RET_FALSE_IFN((consume_digit<UInt32, 2>(ptr, end, part[0])),
707707
"invalid hour offset '{}'", std::string {ptr, end});
708708
}
709-
SET_PARAMS_RET_FALSE_IFN(part[0] <= 14, "invalid hour offset '{}'", part[0]);
709+
SET_PARAMS_RET_FALSE_IFN(
710+
part[0] < (type == DataTimeCastEnumType::TIMESTAMP_TZ ? 24U : 15U),
711+
"invalid hour offset '{}'", part[0]);
710712
if (ptr < end) {
711713
if (*ptr == ':') {
712714
++ptr;
@@ -731,17 +733,15 @@ inline bool CastToDatetimeV2::from_string_strict_mode_internal(
731733
"invalid minute offset '{}'", part[1]);
732734
}
733735
}
734-
SET_PARAMS_RET_FALSE_IFN(part[0] != 14 || (part[1] == 0 && second_offset == 0),
735-
"invalid timezone offset '{}'",
736-
combine_tz_offset(sign, part[0], part[1]));
737-
738-
if (second_offset != 0) {
739-
SET_PARAMS_RET_FALSE_IFN(sign != '-' || part[0] <= 12, "invalid hour offset '{}'",
740-
part[0]);
736+
if constexpr (type == DataTimeCastEnumType::TIMESTAMP_TZ) {
737+
// Wire offsets include historical zones outside the session fixed-zone range.
738+
// Use the exact offset even when it has no seconds (for example, Guam's -14:21).
741739
const auto offset = static_cast<int>(part[0] * 3600 + part[1] * 60 + second_offset);
742740
parsed_tz = cctz::fixed_time_zone(cctz::seconds(sign == '-' ? -offset : offset));
743741
} else {
744-
// Preserve the cached lookup for ordinary minute-aligned offsets.
742+
SET_PARAMS_RET_FALSE_IFN(part[0] != 14 || part[1] == 0,
743+
"invalid timezone offset '{}'",
744+
combine_tz_offset(sign, part[0], part[1]));
745745
SET_PARAMS_RET_FALSE_IFN(
746746
TimezoneUtils::find_cctz_time_zone(
747747
combine_tz_offset(sign, part[0], part[1]), parsed_tz),
@@ -992,7 +992,9 @@ inline bool CastToDatetimeV2::from_string_non_strict_mode_internal(
992992
} else {
993993
PROPAGATE_FALSE((consume_digit<UInt32, 2>(ptr, end, hour_offset)));
994994
}
995-
SET_PARAMS_RET_FALSE_IFN(hour_offset <= 14, "invalid hour offset '{}'", hour_offset);
995+
SET_PARAMS_RET_FALSE_IFN(
996+
hour_offset < (type == DataTimeCastEnumType::TIMESTAMP_TZ ? 24U : 15U),
997+
"invalid hour offset '{}'", hour_offset);
996998
if (ptr < end) {
997999
if (*ptr == ':') {
9981000
++ptr;
@@ -1014,19 +1016,15 @@ inline bool CastToDatetimeV2::from_string_non_strict_mode_internal(
10141016
"invalid minute offset {}", minute_offset);
10151017
}
10161018
}
1017-
SET_PARAMS_RET_FALSE_IFN(
1018-
hour_offset != 14 || (minute_offset == 0 && second_offset == 0),
1019-
"invalid timezone offset '{}'",
1020-
combine_tz_offset(sign, hour_offset, minute_offset));
1021-
1022-
if (second_offset != 0) {
1023-
SET_PARAMS_RET_FALSE_IFN(sign != '-' || hour_offset <= 12,
1024-
"invalid hour offset '{}'", hour_offset);
1019+
if constexpr (type == DataTimeCastEnumType::TIMESTAMP_TZ) {
1020+
// Match strict parsing: a serialized historical offset is not a session setting.
10251021
const auto offset =
10261022
static_cast<int>(hour_offset * 3600 + minute_offset * 60 + second_offset);
10271023
parsed_tz = cctz::fixed_time_zone(cctz::seconds(sign == '-' ? -offset : offset));
10281024
} else {
1029-
// Preserve the cached lookup for ordinary minute-aligned offsets.
1025+
SET_PARAMS_RET_FALSE_IFN(hour_offset != 14 || minute_offset == 0,
1026+
"invalid timezone offset '{}'",
1027+
combine_tz_offset(sign, hour_offset, minute_offset));
10301028
SET_PARAMS_RET_FALSE_IFN(
10311029
TimezoneUtils::find_cctz_time_zone(
10321030
combine_tz_offset(sign, hour_offset, minute_offset), parsed_tz),

‎be/src/exprs/function/cast/cast_to_timestamp_ns.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -382,9 +382,10 @@ class CastToImpl<CastMode, DataTypeTimeStampTz, DataTypeTimeStampNs> : public Ca
382382
col_to->get_data()[i].from_datetime(datetime);
383383
if (!converted) {
384384
if constexpr (CastMode == CastModeType::StrictMode) {
385+
// The session-local year may be unrepresentable even though UTC is valid.
385386
return Status::InvalidArgument(
386387
"can not cast timestamptz {} to TIMESTAMP_NS in timezone {}",
387-
source.to_string(local_time_zone), context->state()->timezone());
388+
source.utc_dt().to_string(source_scale), context->state()->timezone());
388389
}
389390
col_null->get_data()[i] = true;
390391
}

‎be/src/exprs/function/functions_comparison.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -703,10 +703,10 @@ class FunctionComparison : public IFunction {
703703
const auto scale = temporal_type->get_scale();
704704
if (!temporal.to_datetime(local_datetime, context->state()->timezone_obj(), scale,
705705
scale)) [[unlikely]] {
706+
// Preserve the comparison error instead of re-entering the failing formatter.
706707
return Status::InvalidArgument(
707708
"can not compare timestamptz {} with TIMESTAMP_NS in timezone {}",
708-
temporal.to_string(context->state()->timezone_obj(), scale),
709-
context->state()->timezone());
709+
temporal.utc_dt().to_string(scale), context->state()->timezone());
710710
}
711711
comparison = compare_timestamp_ns_with_temporal(timestamp, local_datetime);
712712
} else {

‎be/test/exprs/function/cast/cast_to_timestamptz_test.cpp‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
#include "exprs/function/cast/cast_to_date.h"
3333
#include "exprs/function/cast/cast_to_timestamp_ns.h"
3434
#include "exprs/function/cast/cast_wrapper_decls.h"
35+
#include "exprs/function/functions_comparison.h"
3536
#include "testutil/column_helper.h"
3637
#include "testutil/datetime_ut_util.h"
3738
#include "testutil/mock/mock_runtime_state.h"
@@ -504,4 +505,37 @@ TEST_F(CastTimeStampTzTest, boundary_cast_errors_preserve_status_and_null_semant
504505
}
505506
}
506507

508+
// NOLINTNEXTLINE(readability-function-cognitive-complexity): GTest exception macros add branches.
509+
TEST_F(CastTimeStampTzTest, timestamp_ns_local_year_overflow_returns_status) {
510+
for (const bool upper : {false, true}) {
511+
_state._timezone_obj = cctz::fixed_time_zone(std::chrono::hours(upper ? 8 : -8));
512+
const auto value = upper ? make_timestamptz(9999, 12, 31, 23, 59, 59, 999999)
513+
: make_timestamptz(0, 1, 1, 0, 0, 0, 0);
514+
auto block = ColumnHelper::create_block<DataTypeTimeStampTz>({value});
515+
block.get_by_position(0).type = std::make_shared<DataTypeTimeStampTz>(6);
516+
block.insert({nullptr, std::make_shared<DataTypeTimeStampNs>(), "result"});
517+
CastToImpl<CastModeType::StrictMode, DataTypeTimeStampTz, DataTypeTimeStampNs> cast;
518+
Status status;
519+
ASSERT_NO_THROW(status = cast.execute_impl(&context, block, {0}, 1, 1));
520+
EXPECT_EQ(status.code(), ErrorCode::INVALID_ARGUMENT);
521+
EXPECT_NE(status.to_string().find("can not cast timestamptz"), std::string::npos);
522+
523+
CastToImpl<CastModeType::NonStrictMode, DataTypeTimeStampTz, DataTypeTimeStampNs> try_cast;
524+
ASSERT_TRUE(try_cast.execute_impl(&context, block, {0}, 1, 1).ok());
525+
EXPECT_TRUE(block.get_by_position(1).column->is_null_at(0));
526+
527+
auto ns_column = ColumnTimeStampNs::create();
528+
ns_column->insert_default();
529+
block.get_by_position(1).column = std::move(ns_column);
530+
block.insert({nullptr, std::make_shared<DataTypeUInt8>(), "comparison"});
531+
FunctionComparison<EqualsOp, NameEquals> equals;
532+
// Error reporting must not try to display the unrepresentable session-local year.
533+
for (const ColumnNumbers& inputs : {ColumnNumbers {0, 1}, ColumnNumbers {1, 0}}) {
534+
ASSERT_NO_THROW(status = equals.execute_impl(&context, block, inputs, 2, 1));
535+
EXPECT_EQ(status.code(), ErrorCode::INVALID_ARGUMENT);
536+
EXPECT_NE(status.to_string().find("can not compare timestamptz"), std::string::npos);
537+
}
538+
}
539+
}
540+
507541
} // namespace doris

‎be/test/runtime/timestamptz_value_test.cpp‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,14 @@ TEST(TimeStampTzValueTest, ToStringPreservesHistoricalOffsetSeconds) {
4949
const TestCase cases[] = {
5050
{"Asia/Shanghai", 1890, "1890-01-01 08:05:43", "+08:05:43"},
5151
{"America/New_York", 1880, "1879-12-31 19:03:58", "-04:56:02"},
52+
{.zone = "Asia/Manila",
53+
.year = 1800,
54+
.civil = "1799-12-31 08:03:52",
55+
.offset = "-15:56:08"},
56+
{.zone = "Pacific/Guam",
57+
.year = 1800,
58+
.civil = "1799-12-31 09:39:00",
59+
.offset = "-14:21"},
5260
{"Asia/Shanghai", 2024, "2024-01-01 08:00:00", "+08:00"},
5361
{"America/New_York", 2024, "2023-12-31 19:00:00", "-05:00"},
5462
{"Asia/Kathmandu", 2024, "2024-01-01 05:45:00", "+05:45"},
@@ -196,7 +204,9 @@ TEST(TimeStampTzValueTest, HistoricalOffsetsInStrictAndFallbackParsers) {
196204
for (const std::string input :
197205
{"1890-01-01 08:05:43.123456+08:05:43", "1889-12-31 19:03:58.123456-04:56:02",
198206
"1890-01-01 00:00:30.123456+00:00:30", "1889-12-31 23:59:30.123456-00:00:30",
199-
"1890-01-01 08:05:00.123456+08:05"}) {
207+
"1890-01-01 08:05:00.123456+08:05", "1889-12-31 08:03:52.123456-15:56:08",
208+
"1889-12-31 09:39:00.123456-14:21", "1890-01-01 15:00:00.123456+15:00",
209+
"1889-12-31 11:59:59.123456-12:00:01"}) {
200210
SCOPED_TRACE(input);
201211
for (const bool fallback : {false, true}) {
202212
TimestampTzValue parsed;
@@ -213,7 +223,7 @@ TEST(TimeStampTzValueTest, HistoricalOffsetsInStrictAndFallbackParsers) {
213223
}
214224
}
215225
for (const std::string offset : {"+08:60:00", "+08:05:60", "+08:05:", "+08:05:4", "+08:05:430",
216-
"+14:00:01", "+15:00:00", "-13:00:00"}) {
226+
"+24:00:00", "-24:00:00", "+99:00:00"}) {
217227
SCOPED_TRACE(offset);
218228
const auto input = "1890-01-01 00:00:00" + offset;
219229
for (const bool strict : {false, true}) {

0 commit comments

Comments
 (0)