Copilot commented on code in PR #60913:
URL: https://github.com/apache/doris/pull/60913#discussion_r2867275765
##########
be/src/runtime/runtime_state.h:
##########
@@ -181,6 +182,10 @@ class RuntimeState {
// if possible, use timezone_obj() rather than timezone()
const std::string& timezone() const { return _timezone; }
const cctz::time_zone& timezone_obj() const { return _timezone_obj; }
+ void set_timezone(const std::string& timezone) {
+ _timezone = timezone;
+ TimezoneUtils::find_cctz_time_zone(_timezone, _timezone_obj);
Review Comment:
`RuntimeState::set_timezone()` updates `_timezone` even if
`TimezoneUtils::find_cctz_time_zone()` fails, and it ignores the boolean return
value. If an invalid/unsupported timezone string is passed, `_timezone_obj`
will remain unchanged while `_timezone` changes, leaving the RuntimeState in an
inconsistent state and potentially producing wrong results.
Consider only updating `_timezone` after a successful lookup (or falling
back to `TimezoneUtils::default_time_zone`), and return a `Status`/`bool` so
callers can handle invalid timezones explicitly.
```suggestion
cctz::time_zone tz;
if (TimezoneUtils::find_cctz_time_zone(timezone, tz)) {
_timezone = timezone;
_timezone_obj = tz;
}
```
##########
be/src/service/point_query_executor.cpp:
##########
@@ -319,6 +319,10 @@ Status PointQueryExecutor::init(const
PTabletKeyLookupRequest* request,
*_tablet->tablet_schema(), 1));
}
}
+ // Set timezone from request for functions like from_unixtime()
+ if (request->has_time_zone() && !request->time_zone().empty()) {
+ _reusable->runtime_state()->set_timezone(request->time_zone());
Review Comment:
`PointQueryExecutor::init()` applies `request->time_zone()` to the
cached/reused `RuntimeState` but does not validate whether the timezone string
is supported by BE. With the current `RuntimeState::set_timezone()`
implementation, an unknown timezone can silently leave the timezone object
unchanged.
Once `set_timezone()` reports failure (e.g., via `Status`/`bool`), please
propagate that as an invalid-argument error (or fall back to the default
timezone) so point queries don’t run with a mismatched timezone state.
```suggestion
RETURN_IF_ERROR(_reusable->runtime_state()->set_timezone(request->time_zone()));
```
##########
fe/fe-core/src/main/java/org/apache/doris/qe/PointQueryExecutor.java:
##########
@@ -278,6 +278,12 @@ private RowBatch getNextInternal(Status status, Backend
backend) throws TExcepti
.setOutputExpr(shortCircuitQueryContext.serializedOutputExpr)
.setQueryOptions(shortCircuitQueryContext.serializedQueryOptions)
.setIsBinaryRow(ConnectContext.get().command ==
MysqlCommand.COM_STMT_EXECUTE);
+ // Set timezone for functions like from_unixtime
+ String timeZone =
ConnectContext.get().getSessionVariable().getTimeZone();
+ if ("CST".equals(timeZone)) {
+ timeZone = "Asia/Shanghai";
+ }
+ requestBuilder.setTimeZone(timeZone);
Review Comment:
This introduces a hard-coded timezone string (`"Asia/Shanghai"`) while the
codebase already centralizes the default in `TimeUtils.DEFAULT_TIME_ZONE` (and
uses it in `Coordinator`/`CoordinatorContext` for the same CST aliasing case).
Using the constant here would avoid duplication and keep behavior consistent if
the default ever changes.
--
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]
