github-actions[bot] commented on code in PR #68297:
URL: https://github.com/apache/doris/pull/68297#discussion_r4059255964
##########
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:
[P2] Accept every historical offset emitted here
This now preserves cctz's full historical offset, but both TIMESTAMPTZ
parsers still reject any hour component above 14. For example, current tzdata
gives `Asia/Manila` an offset of `-15:56:08` before 1845, so this formatter
emits that suffix and both strict and fallback parsing reject the returned
value before conversion. Please separate wire-offset parsing from the narrower
session fixed-zone policy (and add a round-trip case for one of these zones) so
every value emitted here can be read back.
##########
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:
[P2] Enforce the fixed-offset endpoint bounds
The new UTC/GMT normalization accepts `UTC+14:30` and `GMT-12:30` because it
only compares the hour, even though this code's fixed-zone contract is
`[-12:00, +14:00]`. The new TIMESTAMPTZ seconds path has the same asymmetry and
accepts `-12:00:01`. Please validate the total offset (or require zero
minutes/seconds at both endpoints) and add boundary cases for the new aliases;
historical wire offsets can be handled separately from session fixed zones.
##########
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:
[P1] Do not exempt identity until the full writer path can carry it
A dynamic identity transform returns the original VARBINARY column, but
`VIcebergTableWriter::_get_iceberg_partition_value` has no `TYPE_VARBINARY`
case and throws; the subsequent `get_partition_value` switch cannot stringify
it either. Even if those are added, FE reconstructs every partitioned file
through `IcebergUtils.parsePartitionValueFromString`, which has no Iceberg
`BINARY`/`FIXED` case. Thus every non-null identity partition still fails
before commit. Please wire one reversible binary representation through BE
extraction/path generation and FE `ByteBuffer` conversion, or reject identity
here, with an end-to-end commit test.
##########
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:
[P1] Accept the static-partition representation FE actually sends
In hybrid-static mode, `VIcebergTableWriter` calls this method with the
string stored in `static_partition_values`, but Nereids
`VarBinaryLiteral.getStringValue()` returns hex such as `00FF` without a `0x`
prefix (and the legacy literal returns raw bytes). Consequently a static
VARBINARY identity component fails this new check before row dispatch. Please
align the FE/BE representation or accept the producer's actual encoding, and
cover the writer call chain rather than only constructing `0x...` directly in
the SerDe unit test.
##########
be/src/exec/common/hash_table/hash_key_type.h:
##########
@@ -102,6 +102,13 @@ inline HashKeyType get_hash_key_type_fixed(const
std::vector<DataTypePtr>& data_
}
inline HashKeyType get_hash_key_type(const std::vector<DataTypePtr>&
data_types) {
+ // Reject binary before the multi-key serialization fallback can enable
joins or grouping.
+ for (const auto& type : data_types) {
+ if (type->get_primitive_type() == TYPE_VARBINARY) {
Review Comment:
[P2] Reject VARBINARY recursively before serialized hashing
This check only sees the outer primitive type, so keys such as
`ARRAY<VARBINARY>` and `STRUCT<VARBINARY>` fall through to
`HashKeyType::serialized`; `SerializedHashTableContext` then serializes and
hashes the whole complex column. The matching Nereids GROUP BY checks are also
top-level-only. That re-enables the grouping/join computation this fence is
intended to keep unsupported. Please reject VARBINARY anywhere in the key type
tree (in both FE and BE), and cover nested single- and multi-key cases.
##########
be/src/exprs/function/in.h:
##########
@@ -105,6 +105,10 @@ class FunctionIn : public IFunction {
if (scope == FunctionContext::THREAD_LOCAL) {
return Status::OK();
}
+ // Binary IO must not route IN through the shared string/storage
predicate implementation.
+ if (context->get_arg_type(0)->get_primitive_type() == TYPE_VARBINARY) {
Review Comment:
[P2] Reject nested VARBINARY before collection IN
This guard only runs for the scalar `in` function. An `ARRAY<VARBINARY>`
operand passes Nereids' recursive comparability check, and `VInPredicate`
routes the complex outer type to `collection_in` instead, bypassing this code.
During fragment open that function hashes each constant through
`ColumnArray::update_crc_with_value`, which reaches `ColumnVarbinary`'s
inherited unsupported implementation and fails the query. Please reject
VARBINARY recursively before choosing scalar or collection IN (with
ARRAY/STRUCT, nullable, and NOT IN coverage).
##########
be/src/exprs/aggregate/aggregate_function_min_max_impl.h:
##########
@@ -140,6 +140,10 @@ AggregateFunctionPtr
create_aggregate_function_single_value(const String& name,
return creator_without_type::create_unary_arguments<
AggregateFunctionsSingleValue<Data<SingleValueDataComplexType>>>(
argument_types, result_is_nullable, attr);
+ case PrimitiveType::TYPE_VARBINARY:
Review Comment:
[P2] Apply the aggregate ban to nested VARBINARY too
The complex-type cases above run before this direct VARBINARY rejection, so
`min`/`max(ARRAY<VARBINARY>)` constructs `SingleValueDataComplexType`. Its
comparison recursively reaches `ColumnVarbinary::compare_at`, enabling the
ordering this change is trying to keep unsupported. The separate
`min_by`/`max_by` factory has the same complex-key dispatch, so fixing only
this switch leaves that sibling bypass open. Please reject VARBINARY
recursively in both factories and add nested-array cases, while leaving
byte-preserving aggregates such as `collect_list` alone.
--
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]
