github-actions[bot] commented on code in PR #67064:
URL: https://github.com/apache/doris/pull/67064#discussion_r3842938364
##########
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);
} else {
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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/StringLikeLiteral.java:
##########
@@ -141,11 +140,7 @@ protected Expression uncheckedCastTo(DataType targetType)
throws AnalysisExcepti
if (timeStampTzType.getScale() < 0) {
timeStampTzType = TimeStampTzType.forTypeFromString(value);
}
- if (DateTimeChecker.hasTimeZone(value)) {
- return new TimestampTzLiteral(timeStampTzType, value);
- }
- DateTimeV2Literal datetime = (DateTimeV2Literal)
castToDateTime(DateTimeV2Type.MAX, strictCast);
- return TimestampTzLiteral.fromSessionTimeZone(timeStampTzType,
datetime);
+ return castToDateTime(timeStampTzType, strictCast);
Review Comment:
[P1] Preserve trailing whitespace on the new fold path
In strict mode, `cast('2026-01-01 00:00:00 -08:00 ' as timestamptz(6))` now
reaches `castToDateTime`, whose strict regex runs `matches()` on the raw value
and has no trailing-whitespace suffix. The old explicit-zone constructor path
trimmed through timezone detection/date parsing, and BE strict parsing still
skips trailing whitespace after the offset. Consequently the folded query
throws during analysis while `debug_skip_fold_constant=true` succeeds. Please
apply the offset validator without narrowing the accepted outer-whitespace
grammar (or trim the same permitted whitespace before matching), and add a
strict fold/runtime parity case including `-14:00`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java:
##########
@@ -675,7 +674,7 @@ public static Optional<Expression>
characterLiteralTypeCoercion(String value, Da
if (timeStampTzType.getScale() < 0 ||
timeStampTzType.getScale() == TimeStampTzType.MAX_SCALE) {
timeStampTzType = TimeStampTzType.forTypeFromString(value);
}
- ret = TimestampTzLiteral.fromSessionTimeZone(timeStampTzType,
value);
+ ret = new StringLiteral(value).checkedCastTo(timeStampTzType);
Review Comment:
[P1] Keep TIMESTAMPTZ predicates in the instant domain
A reduced trigger is:
```text
Filter(ts = '2024-11-03:01:30:00')
Scan(ts TIMESTAMPTZ)
```
`DateTimeChecker` and the BE strict parser accept the colon date/time
delimiter, but the new `checkedCastTo` route rejects it because
`StringLikeLiteral`'s strict regex permits only space or `T`.
`characterLiteralTypeCoercion` then returns empty, and common-type selection
rewrites the predicate to:
```text
Filter(Cast(ts AS DATETIMEV2(6)) = Cast(string AS DATETIMEV2(6)))
```
In `America/New_York`'s fall-back overlap, the 05:30Z and 06:30Z TIMESTAMPTZ
values both become local 01:30, so skipped folding can match both instead of
the single pre-transition instant; normal folding can instead raise a
strict-cast error. Please preserve the TIMESTAMPTZ target when eager parsing
declines, or share only the offset validator without routing through the
narrower grammar, and assert the final common type in a DST-overlap
comparison/IN test.
##########
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.
--
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]
