github-actions[bot] commented on code in PR #67174:
URL: https://github.com/apache/doris/pull/67174#discussion_r3863217002
##########
be/src/runtime/runtime_state.h:
##########
@@ -225,9 +226,14 @@ 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) {
+ Status set_timezone(const std::string& timezone) {
Review Comment:
[P1] Consume the new Status at every setter call
`Status` itself is `[[nodiscard]]`, and BE compiles with `-Werror`, so
changing this setter from `void` leaves the 56 existing test calls that still
do `state.set_timezone(...)` as `-Wunused-result` build failures. This includes
calls in changed `be/test/format/table/iceberg/iceberg_reader_test.cpp` as well
as the format-v2 ORC, Parquet, JNI, Hudi, and transformer tests. Please update
every caller in this PR to consume/assert/propagate the result; the production
point-query caller already uses `RETURN_IF_ERROR`.
##########
be/test/core/data_type_serde/data_type_serde_arrow_test.cpp:
##########
@@ -289,7 +289,7 @@ std::shared_ptr<Block>
create_test_block(std::vector<PrimitiveType> cols, int ro
DateV2Value<DateTimeV2ValueType> value;
std::string date_literal = "2022-01-01 11:11:11.111";
cctz::time_zone ctz;
- TimezoneUtils::find_cctz_time_zone("UTC", ctz);
+ ASSERT_TRUE(TimezoneUtils::find_cctz_time_zone("UTC", ctz));
Review Comment:
[P1] Do not use a fatal assertion in this non-void helper
`create_test_block()` returns `std::shared_ptr<Block>`, but `ASSERT_TRUE`
expands to a fatal void return and therefore cannot compile here. The same
mechanical replacement appears in four other non-void helpers:
`parquet_expr_test.cpp:477`, `hive_reader_create_column_ids_test.cpp:662/702`,
and `iceberg_reader_create_column_ids_test.cpp:692`. Please use a non-fatal
check where continuing is safe, or explicitly record the failure and return a
value of the helper's declared type.
##########
be/src/format/table/es/es_scroll_parser.cpp:
##########
@@ -214,8 +214,8 @@ Status get_date_value_int(const rapidjson::Value& col,
PrimitiveType type, bool
fmt = "%Y-%m-%dT%H:%M:%E*S%Ez";
cctz::time_zone ctz;
// find time_zone by time_zone suffix string
- TimezoneUtils::find_cctz_time_zone(value.as_string(), ctz);
- ok = cctz::parse(fmt, str_date, ctz, &tp);
+ ok = TimezoneUtils::find_cctz_time_zone(value.as_string(),
ctz) &&
Review Comment:
[P1] Preserve the ES embedded-offset grammar
This branch deliberately accepts embedded offsets such as `+0900` and
`+23:59` (the existing ES parser tests preserve both), but
`find_cctz_time_zone()` only accepts Doris session-zone spellings and ranges.
The new short-circuit therefore rejects valid ES values before CCTZ's `%Ez`
parser, which accepts the optional colon and applies the embedded offset
independently of the supplied zone. Please parse with UTC or use a dedicated
embedded-offset validator that preserves this grammar, and add an end-to-end
value parse test.
##########
be/src/runtime/runtime_state.cpp:
##########
@@ -249,7 +249,7 @@ RuntimeState::RuntimeState(const TQueryOptions&
query_options, const TQueryGloba
Status status = init(TUniqueId(), query_options, query_globals, nullptr);
_exec_env = ExecEnv::GetInstance();
init_mem_trackers("<unnamed>");
- DCHECK(status.ok());
+ DORIS_CHECK(status.ok()) << status;
Review Comment:
[P1] Initialize named zones in the BE unit-test process
The BE-UT main never calls `TimezoneUtils::load_timezones_to_cache()`, but
this new unconditional check makes filtered `FunctionUtils` users abort when
their `TQueryGlobals` contains `Asia/Shanghai`. The same missing preload makes
JNI/Hudi/cast/Iceberg `set_timezone("America/...")` calls return
`InvalidArgument` and retain `+08:00`, and makes direct v1 ORC tests using
`CST` abort in their new constructor checks. Please preload named zones in the
shared BE-UT setup (and consume/assert setter statuses) so these tests remain
independently runnable instead of depending on an unrelated earlier suite.
--
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]
