Gabriel39 commented on code in PR #68297:
URL: https://github.com/apache/doris/pull/68297#discussion_r4060819325
##########
be/src/core/value/timestamptz_value.cpp:
##########
@@ -65,6 +73,14 @@ std::string TimestampTzValue::to_string(const
cctz::time_zone& tz, int scale) co
buffer[len++] = ':';
buffer[len++] = static_cast<char>('0' + offset_mins / 10);
buffer[len++] = '0' + offset_mins % 10;
+ // Historical zones can have sub-minute offsets. Dropping their seconds
changes the
Review Comment:
Fixed in 7684c0cfd377c77399f18bac0ffaefc88540e56a. Both strict and fallback
TIMESTAMPTZ parsers now interpret historical wire offsets independently of the
narrower session fixed-zone policy. Tests cover Manila (-15:56:08), Guam
(-14:21), positive offsets beyond +14:00, and malformed offsets. DATE/DATETIME
parsing is retained; this completes the exact-offset round-trip contract
introduced by this PR.
##########
be/src/util/timezone_utils.cpp:
##########
@@ -154,33 +158,125 @@ bool TimezoneUtils::find_cctz_time_zone(const
std::string& timezone, cctz::time_
ctz = it->second;
return true;
}
- // V2 readers and Iceberg writers may resolve UTC/fixed offsets before
ExecEnv preloads the
- // timezone cache, so retain the cache fast path but handle those
self-contained zones here.
- const auto normalized = to_lower_copy(timezone);
- if (normalized == "utc" || normalized == "etc/utc" || normalized ==
"zulu") {
- ctz = cctz::utc_time_zone();
+
+ std::string normalized;
+ if (!normalize_timezone_name(timezone, &normalized)) {
+ return false;
+ }
+ if (auto it = lower_zone_cache_->find(to_lower_copy(normalized));
+ it != lower_zone_cache_->end()) [[likely]] {
+ ctz = it->second;
return true;
}
- return parse_tz_offset_string(timezone, ctz);
+ return parse_tz_offset_string(normalized, ctz);
}
-bool TimezoneUtils::parse_tz_offset_string(const std::string& timezone,
cctz::time_zone& ctz) {
- // like +08:00, which not in timezone_names_map_
- re2::StringPiece value;
- if (time_zone_offset_format_reg.Match(timezone, 0, timezone.size(),
RE2::UNANCHORED, &value, 1))
- [[likely]] {
- bool positive = value[0] != '-';
+bool TimezoneUtils::try_get_fixed_offset_seconds(const cctz::time_zone&
timezone,
+ int32_t* offset_seconds) {
+ DORIS_CHECK(offset_seconds != nullptr);
+ const std::string& timezone_name = timezone.name();
+ if (timezone_name == "UTC" || timezone_name == "Etc/UTC" || timezone_name
== "Etc/GMT") {
+ *offset_seconds = 0;
+ return true;
+ }
+
+ // cctz names fixed_time_zone() instances with the "Fixed/" prefix. TZDB's
Etc/GMT*
+ // zones are fixed offsets too; cctz handles their POSIX-style reversed
sign in lookup_offset().
+ // If this naming convention changes, falling through to the generic path
remains correct.
+ static const auto epoch = std::chrono::time_point_cast<cctz::sys_seconds>(
+ std::chrono::system_clock::from_time_t(0));
+ if (timezone_name.compare(0, 6, "Fixed/") == 0 || timezone_name.compare(0,
7, "Etc/GMT") == 0) {
+ *offset_seconds = timezone.lookup_offset(epoch).offset;
+ return true;
+ }
+ return false;
+}
+
+static bool normalize_offset_string(const std::string& timezone, bool
allow_hour_only,
+ std::string* normalized) {
+ if (timezone.size() < 2 || (timezone[0] != '+' && timezone[0] != '-')) {
+ return false;
+ }
+
+ const bool positive = timezone[0] == '+';
+ const std::string_view rest(timezone.data() + 1, timezone.size() - 1);
+ int hour = 0;
+ int minute = 0;
- //Regular expression guarantees hour and minute must be int
- int hour = std::stoi(value.substr(1, 2).as_string());
- int minute = std::stoi(value.substr(4, 2).as_string());
+ const auto parse_digit = [](char c) -> int { return c - '0'; };
+ const auto is_two_digits = [](std::string_view value) -> bool {
+ return value.size() == 2 && std::isdigit(static_cast<unsigned
char>(value[0])) &&
+ std::isdigit(static_cast<unsigned char>(value[1]));
+ };
+ const auto is_one_or_two_digits = [](std::string_view value) -> bool {
+ return (value.size() == 1 || value.size() == 2) &&
+ std::all_of(value.begin(), value.end(),
+ [](char c) { return
std::isdigit(static_cast<unsigned char>(c)); });
+ };
- // timezone offsets around the world extended from -12:00 to +14:00
- if (!positive && hour > 12) {
+ const auto colon_pos = rest.find(':');
+ if (colon_pos != std::string_view::npos) {
+ const std::string_view hour_part = rest.substr(0, colon_pos);
+ const std::string_view minute_part = rest.substr(colon_pos + 1);
+ if (!is_one_or_two_digits(hour_part) || !is_two_digits(minute_part)) {
return false;
- } else if (positive && hour > 14) {
+ }
+ hour = std::stoi(std::string(hour_part));
+ minute = parse_digit(minute_part[0]) * 10 +
parse_digit(minute_part[1]);
+ } else {
+ if (!allow_hour_only || !is_one_or_two_digits(rest)) {
+ return false;
+ }
+ hour = std::stoi(std::string(rest));
+ }
+
Review Comment:
Fixed in 7684c0cfd377c77399f18bac0ffaefc88540e56a. The new normalization
validates the complete fixed offset, including endpoint minutes, and rejected
offsets are not inserted into the cache as UTC. Tests cover direct parsing and
cached lookup of UTC/GMT aliases and bare offsets. Historical TIMESTAMPTZ wire
offsets are deliberately parsed separately, so -12:00:01 is valid on the wire
rather than being subject to the session-setting range.
##########
be/src/core/data_type_serde/data_type_varbinary_serde.cpp:
##########
@@ -301,6 +305,82 @@ Status
DataTypeVarbinarySerDe::deserialize_one_cell_from_json(IColumn& column, S
return Status::OK();
}
+Status DataTypeVarbinarySerDe::from_string(StringRef& str, IColumn& column,
+ const FormatOptions& options) const
{
+ // Partition structs use the same hex representation as nested VARBINARY
output. Decode it
+ // before appending so arbitrary bytes survive JSON transport instead of
becoming NULL.
Review Comment:
The pre-PR VARBINARY SerDe returned NotSupported from from_string(), and the
complete binary identity partition writer/commit path was already unsupported.
Adding static-partition encoding support would broaden this PR.
##########
be/src/exec/sink/writer/iceberg/partition_transformers.cpp:
##########
@@ -46,6 +46,12 @@ const std::chrono::sys_days
PartitionColumnTransformUtils::EPOCH = std::chrono::
std::unique_ptr<PartitionColumnTransform> PartitionColumnTransforms::create(
const doris::iceberg::PartitionField& field, const DataTypePtr&
source_type) {
auto& transform = field.transform();
+ // Identity/void only carry values; computed binary partition transforms
are unsupported.
+ if (source_type->get_primitive_type() == TYPE_VARBINARY && transform !=
"identity" &&
Review Comment:
The binary identity extraction, path generation, and FE commit
reconstruction gaps predate this PR. This follow-up does not implement binary
partition writing or redesign its validation 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]
