github-actions[bot] commented on code in PR #67064:
URL: https://github.com/apache/doris/pull/67064#discussion_r3842944798
##########
be/src/exprs/function/cast/cast_base.cpp:
##########
@@ -17,9 +17,24 @@
#include "exprs/function/cast/cast_base.h"
+#include <cctz/time_zone.h>
+
#include <cstdint>
#include "util/jsonb_writer.h"
+
+namespace doris::CastUtil {
+
+void make_fixed_time_zone(char sign, uint32_t hour, uint32_t minute,
cctz::time_zone& result) {
+ int offset = static_cast<int>((hour * 60 + minute) * 60);
+ if (sign == '-') {
+ offset = -offset;
+ }
+ result = cctz::fixed_time_zone(cctz::seconds(offset));
Review Comment:
[P1] Keep fixed-zone lookup off the row hot path
Every nonzero bare offset now calls `cctz::fixed_time_zone` for each parsed
row in all six CAST branches. In the pinned cctz v2.5, [`fixed_time_zone` calls
`load_time_zone`](https://github.com/google/cctz/blob/v2.5/src/time_zone_lookup.cc#L174-L180),
and even a cached hit [takes the process-wide
`TimeZoneMutex`](https://github.com/google/cctz/blob/v2.5/src/time_zone_impl.cc#L44-L75).
The old `lower_zone_cache_` fast path copied an already-loaded zone without
that lock, so ordinary `+08:00` columns now acquire a global mutex once per row
and concurrent scans/loads serialize there. Please keep the expanded CAST range
in a finite CAST-local read-only cache (without exposing it to generic timezone
lookup) so row parsing only copies a cached zone.
##########
be/src/exprs/function/cast/cast_to_date_or_datetime_impl.hpp:
##########
@@ -671,10 +671,7 @@ inline bool
CastToDateOrDatetime::from_string_strict_mode(const StringRef& str,
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]));
+ CastUtil::make_fixed_time_zone(sign, part[0], part[1], parsed_tz);
Review Comment:
[P1] Reject DATETIME overflow after applying the new offset
With session UTC, `cast('9999-12-31 11:30:00 -13:00' as datetime)` now
reaches this helper and converts to `10000-01-01 00:30:00`. Both
legacy-DATETIME parser blocks then publish `local.year()` through
`unchecked_set_time_unit` and return success without the post-conversion `year
<= 9999` check used by DATETIMEV2/TIMESTAMPTZ. FE folding rejects the same
result, while runtime stores an invalid `VecDateTimeValue` whose formatter
assumes four year digits. This trigger is introduced by the expanded range:
pre-PR lookup rejects `-13:00`, and `-12:00` at this time stays within 9999.
Please range-check after timezone conversion in both legacy paths and add
strict/non-strict fold/runtime coverage at the upper-year boundary.
--
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]
