Copilot commented on code in PR #51454:
URL: https://github.com/apache/doris/pull/51454#discussion_r2163216032
##########
be/src/vec/runtime/vdatetime_value.cpp:
##########
@@ -2185,40 +2185,29 @@ bool DateV2Value<T>::from_date_str_base(const char*
date_str, int len, int scale
if (!TimezoneUtils::find_cctz_time_zone(std::string {ptr, end},
given_tz)) {
return false; // invalid format
}
- auto given = cctz::convert(cctz::civil_second {}, given_tz);
- auto local = cctz::convert(cctz::civil_second {}, *local_time_zone);
- // these two values is absolute time. so they are negative. need to
use (-local) - (-given)
- sec_offset = std::chrono::duration_cast<std::chrono::seconds>(given -
local).count();
- }
-
- // In check_range_and_set_time, for Date type the time part will be
truncated. So if the timezone offset should make
- // rounding to date part, it would be lost. To avoid this, we use a
Datetime type to do these calc. It will save the
- // time part and apply the offset. Then convert to Date type back.
- // see https://github.com/apache/doris/pull/33553 for more details.
- if constexpr (!is_datetime) {
- if (sec_offset) {
- DateV2Value<DateTimeV2ValueType> tmp;
- if (!tmp.check_range_and_set_time(date_val[0], date_val[1],
date_val[2], date_val[3],
- date_val[4], date_val[5],
date_val[6])) {
- return false;
- }
- if (!tmp.date_add_interval<TimeUnit::SECOND>(
- TimeInterval {TimeUnit::SECOND, sec_offset, false})) {
- return false;
- }
- this->assign_from(tmp);
- return true;
+ if (is_invalid(date_val[0], date_val[1], date_val[2], date_val[3],
date_val[4], date_val[5],
+ date_val[6])) {
+ return false;
}
- }
-
- if (!check_range_and_set_time(date_val[0], date_val[1], date_val[2],
date_val[3], date_val[4],
- date_val[5], date_val[6])) {
- return false;
- }
-
- return sec_offset ? date_add_interval<TimeUnit::SECOND>(
- TimeInterval {TimeUnit::SECOND, sec_offset,
false})
- : true;
+ // will carring on the bits in cctz::civil_second. if day is 70, will
carry to month.
+ cctz::civil_second cs {date_val[0], date_val[1], date_val[2],
+ date_val[3], date_val[4], date_val[5]};
+
+ auto given = cctz::convert(cs, given_tz);
+ auto local = cctz::convert(given, *local_time_zone);
+ date_val[0] = local.year();
+ date_val[1] = local.month();
+ date_val[2] = local.day();
+ date_val[3] = local.hour();
+ date_val[4] = local.minute();
+ date_val[5] = local.second();
+ }
+
+ return check_range_and_set_time(date_val[0], date_val[1], date_val[2],
date_val[3], date_val[4],
+ date_val[5], date_val[6]) &&
+ (sec_offset ? date_add_interval<TimeUnit::SECOND>(
+ TimeInterval {TimeUnit::SECOND, sec_offset,
false})
+ : true);
Review Comment:
The variable 'sec_offset' is declared and initialized to 0 but is never
updated due to the removed block that computed its value, making the
conditional check redundant. Consider removing 'sec_offset' and simplifying the
return statement to directly use the result of check_range_and_set_time.
--
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]
