Skip to content

[fix](types) Fix binary value ownership and timestamp primitives #68301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yiguolei merged 6 commits into from
Sep 22, 2026
Merged

[fix](types) Fix binary value ownership and timestamp primitives #68301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b2bb9ea
[fix](types) Fix binary value ownership and timestamp primitives
Gabriel39 Sep 21, 2026
44eef01
[refactor](nereids) Validate binary collection arguments in functions
Gabriel39 Sep 21, 2026
f68c0ba
[fix](regression) Update historical TIMESTAMPTZ offset expectations
Gabriel39 Sep 21, 2026
0765475
[fix](types) Close timestamp wire parsing and error-reporting gaps
Gabriel39 Sep 21, 2026
74fc447
[fix](test) Align binary literal and timestamp offset checks
Gabriel39 Sep 21, 2026
221207e
[fix](be) Track binary Field payloads and stabilize offset tests
Gabriel39 Sep 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions be/src/core/column/column_varbinary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,26 @@
#include "exec/sort/sort_block.h"

namespace doris {

void ColumnVarbinary::insert_many_continuous_binary_data(const char* data, const uint32_t* offsets,
size_t num) {
reserve(size() + num);

@yiguolei yiguolei Sep 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个reserve 是错的。 不是 +num，应该是计算一下offsets【num】 - 0 ？

for (size_t row = 0; row < num; ++row) {
insert_data(data + offsets[row], offsets[row + 1] - offsets[row]);
}
}

void ColumnVarbinary::insert_many_dict_data(const int32_t* data_array, size_t start_index,
const StringRef* dict, size_t data_num,
uint32_t dict_num) {
reserve(size() + data_num);
// Decoder pages can be released after the call; copy long dictionary entries into our arena.
for (size_t row = start_index; row < start_index + data_num; ++row) {
const auto& value = dict[data_array[row]];
insert_data(value.data, value.size);
}
}

MutableColumnPtr ColumnVarbinary::clone_resized(size_t size) const {
auto res = create();
if (size > 0) {
Expand Down
6 changes: 6 additions & 0 deletions be/src/core/column/column_varbinary.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "core/string_view.h"

namespace doris {
// Binary IO does not enable hash computation; inherit IColumn's unsupported methods.
class ColumnVarbinary final : public COWHelper<IColumn, ColumnVarbinary> {
private:
using Self = ColumnVarbinary;
Expand Down Expand Up @@ -189,6 +190,11 @@ class ColumnVarbinary final : public COWHelper<IColumn, ColumnVarbinary> {
void insert_many_strings_overflow(const StringRef* strings, size_t num,
size_t max_length) override;

void insert_many_continuous_binary_data(const char* data, const uint32_t* offsets,
size_t num) override;
void insert_many_dict_data(const int32_t* data_array, size_t start_index, const StringRef* dict,
size_t data_num, uint32_t dict_num = 0) override;

void sort_column(const ColumnSorter* sorter, EqualFlags& flags, IColumn::Permutation& perms,
EqualRange& range, bool last_column) const override;

Expand Down
4 changes: 4 additions & 0 deletions be/src/core/data_type/data_type_factory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,6 +635,10 @@ DataTypePtr DataTypeFactory::create_data_type(
} else if (primitive_type == TYPE_AGG_STATE) {
// Do nothing
nested = std::make_shared<DataTypeAggState>();
} else if (primitive_type == TYPE_VARBINARY) {

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Complete execution support for this newly reconstructible type in the default conditional evaluators. With the default short_circuit_evaluation=false, a multi-row coalesce whose selected values come from different arguments reaches filled_result_column, where dispatch_switch_scalar omits VARBINARY; a two-or-more-WHEN CASE separately reaches VCaseExpr::_execute_update_result, whose type switch also omits it. Both expressions are FE-legal, and both work when short-circuit evaluation is enabled through generic insert_from, so results currently depend on this session setting. Please add the generic/VARBINARY paths and regression cases for mixed-row COALESCE and multi-branch CASE under both settings.

@Gabriel39 Gabriel39 Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The normal COALESCE/CASE dispatch omissions and the alternate short-circuit paths predate this PR. The protobuf datatype reconstruction change does not introduce those evaluator branches. Adding VARBINARY execution support is outside this follow-up.

// Serialized execution types must retain VARBINARY(n)'s byte limit across RPCs.
return create_data_type(primitive_type, is_nullable, 0, 0,
scalar_type.has_len() ? scalar_type.len() : -1);
} else if (primitive_type == TYPE_VARIANT) {
nested = std::make_shared<DataTypeVariantV2>(node.variant_max_subcolumns_count(),
node.variant_enable_doc_mode());
Expand Down
80 changes: 80 additions & 0 deletions be/src/core/data_type_serde/data_type_varbinary_serde.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,17 @@
#include "core/data_type_serde/data_type_varbinary_serde.h"

#include <cstring>
#include <limits>

#include "common/config.h"
#include "core/column/column_varbinary.h"
#include "core/data_type_serde/arrow_validation.h"
#include "core/data_type_serde/parquet_decode_source.h"
#include "exprs/function/string_hex_util.h"
#include "util/url_coding.h"

namespace doris {

namespace {

class VarbinaryParquetConsumer final : public ParquetFixedValueConsumer,
Expand Down Expand Up @@ -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.

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Preserve the top-level partition-string contract here. With enable.mapping.varbinary=true, HiveScanRange copies each non-null HMS partition value directly into columns_from_path, and FileScannerV2::_parse_partition_value calls this from_string; any ordinary value not starting with 0x now fails with Invalid VARBINARY hex representation. The 0x grammar is symmetric only with nested serialization (to_string emits it at nesting level >= 2), so please restrict hex decoding to that context or use a dedicated nested decoder, and add a Hive partition scan test.

@Gabriel39 Gabriel39 Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The top-level and nested encodings differ, but the pre-PR VARBINARY SerDe inherited from_string() returning NotSupported. Thus this is not a regression of a previously supported Hive partition decoder. Adding that decoder is outside the current primitive changes.

if (str.size < 2 || str.data[0] != '0' || str.data[1] != 'x' || (str.size - 2) % 2 != 0 ||
str.size - 2 > std::numeric_limits<int>::max()) {
return Status::InvalidArgument("Invalid VARBINARY hex representation");
}
// The INT_MAX guard also makes narrowing to the decoder's 32-bit offset type safe.
const auto hex_size = cast_set<ColumnString::Offset>(str.size - 2);
std::string bytes(hex_size / 2, '\0');
if (string_hex::hex_decode(str.data + 2, hex_size, bytes.data()) != bytes.size()) {
return Status::InvalidArgument("Invalid VARBINARY hex representation");
}
assert_cast<ColumnVarbinary&>(column).insert_data(bytes.data(), bytes.size());
return Status::OK();
}

Status DataTypeVarbinarySerDe::deserialize_one_cell_from_hive_text(

@github-actions github-actions Bot Sep 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Propagate the Hive BINARY decoding contract instead of guessing it from each cell. Hive 4 supports hive.serialization.decode.binary.as.base64=false for raw TEXTFILE values, but FE drops that property, so a configured raw value such as test is silently Base64-decoded here to different bytes. In default mode Hive 4/current LazyBinary also uses the strict Basic decoder with raw fallback, while this code accepts URL-safe letters/whitespace and truncates a one-sextet tail (for example Hive preserves -_8= but Doris returns 0xfb 0xff). Please pass the property/version-selected mode through TFileAttributes, use the matching decoder, and cover raw plus Hive 3/4 Base64 cases. See the Hive TEXTFILE contract and Hive 4 LazyBinary.

IColumn& column, Slice& slice, const FormatOptions& options,
int hive_text_complex_type_delimiter_level) const {
// Hive LazyBinary uses lenient Base64 (including URL-safe letters and whitespace),
// falling back to the original bytes for non-Base64 input or an empty decoding.
// Keep this separate from JSON/CSV: those formats do not share Hive's encoding contract.
std::string encoded;

@github-actions github-actions Bot Sep 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Put these full-cell Hive staging buffers behind Doris's checked allocator. For every valid binary field this reserves slice.size in encoded, base64_decode then resizes a second std::string to the same size, and ColumnVarbinary finally copies the decoded bytes into its arena. Ordinary std::string storage does not run the Allocator<false> admission check, so one large external field can build roughly two input-sized buffers before the tracked column allocation rejects it; the new write path likewise materializes both value.to_string() and its Base64 output. Please decode/encode directly into checked destination storage (and avoid the hex path's equivalent whole-cell temporary), with a limited-memory scan/write test.

encoded.reserve(slice.size);
bool padding = false;
for (size_t i = 0; i < slice.size; ++i) {
const char c = slice.data[i];
if (c == ' ' || c == '\t' || c == '\r' || c == '\n') {
continue;
}
if (c == '=') {
padding = true;
continue;
}
if (!((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') ||
c == '+' || c == '/' || c == '-' || c == '_')) {
return deserialize_one_cell_from_json(column, slice, options);
}
if (!padding) {
encoded.push_back(c == '-' ? '+' : c == '_' ? '/' : c);
}
}
// Commons Codec ignores a trailing sextet and accepts omitted padding.
if (encoded.size() % 4 == 1) {
encoded.pop_back();
}
encoded.append((4 - encoded.size() % 4) % 4, '=');
std::string decoded;
if (!base64_decode(encoded, &decoded) || decoded.empty()) {
return deserialize_one_cell_from_json(column, slice, options);
}
assert_cast<ColumnVarbinary&>(column).insert_data(decoded.data(), decoded.size());
return Status::OK();
}

Status DataTypeVarbinarySerDe::deserialize_column_from_hive_text_vector(
IColumn& column, std::vector<Slice>& slices, uint64_t* num_deserialized,
const FormatOptions& options, int hive_text_complex_type_delimiter_level) const {
DESERIALIZE_COLUMN_FROM_HIVE_TEXT_VECTOR()
return Status::OK();
}

Status DataTypeVarbinarySerDe::serialize_one_cell_to_hive_text(
const IColumn& column, int64_t row_num, BufferWritable& bw, FormatOptions& options,
int hive_text_complex_type_delimiter_level) const {
auto [data_column, data_row] = check_column_const_set_readability(column, row_num);
const auto value = assert_cast<const ColumnVarbinary&>(*data_column).get_data_at(data_row);
// Encoding is required on write as well, or a Hive reader will reinterpret binary bytes.
std::string encoded;
base64_encode(value.to_string(), &encoded);
bw.write(encoded.data(), encoded.size());
return Status::OK();
}

void DataTypeVarbinarySerDe::to_string(const IColumn& column, size_t row_num, BufferWritable& bw,
const FormatOptions& options) const {
const auto& value = assert_cast<const ColumnVarbinary&>(column).get_data()[row_num];
Expand Down
16 changes: 16 additions & 0 deletions be/src/core/data_type_serde/data_type_varbinary_serde.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,19 @@ class DataTypeVarbinarySerDe : public DataTypeSerDe {
Status deserialize_one_cell_from_json(IColumn& column, Slice& slice,
const FormatOptions& options) const override;

Status deserialize_one_cell_from_hive_text(
IColumn& column, Slice& slice, const FormatOptions& options,
int hive_text_complex_type_delimiter_level = 1) const override;

Status deserialize_column_from_hive_text_vector(
IColumn& column, std::vector<Slice>& slices, uint64_t* num_deserialized,
const FormatOptions& options,
int hive_text_complex_type_delimiter_level = 1) const override;

Status serialize_one_cell_to_hive_text(
const IColumn& column, int64_t row_num, BufferWritable& bw, FormatOptions& options,
int hive_text_complex_type_delimiter_level = 1) const override;

Status deserialize_column_from_json_vector(IColumn& column, std::vector<Slice>& slices,
uint64_t* num_deserialized,
const FormatOptions& options) const override {
Expand Down Expand Up @@ -95,6 +108,9 @@ class DataTypeVarbinarySerDe : public DataTypeSerDe {

void to_string(const IColumn& column, size_t row_num, BufferWritable& bw,
const FormatOptions& options) const override;

Status from_string(StringRef& str, IColumn& column,
const FormatOptions& options) const override;
};

} // namespace doris
73 changes: 71 additions & 2 deletions be/src/core/field.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

#include "common/compare.h"
#include "core/accurate_comparison.h"
#include "core/allocator.h"
#include "core/allocator_fwd.h"
#include "core/data_type/data_type_decimal.h"
#include "core/data_type/define_primitive_type.h"
#include "core/data_type/primitive_type.h"
Expand Down Expand Up @@ -91,6 +93,50 @@ bool decimal_less_or_equal(Decimal128V3 x, Decimal128V3 y, UInt32 xs, UInt32 ys)
return dec_less_or_equal<TYPE_DECIMAL128I>(x, y, xs, ys);
}

namespace {
// Expression literals can outlive decoder pages and source columns.
// Keep the view first for Field::get(), and fit ownership into the existing Field storage.
struct OwnedBinaryField {
StringView view;
char* bytes = nullptr;
size_t byte_size = 0;

explicit OwnedBinaryField(const StringView& value) {
// Inline views already own their bytes; preserve their allocation-free representation.
if (value.isInline()) {
view = value;
return;
}
// Charge retained payloads and deep-copy peaks through Doris's checked allocator.
// Keep a standard-layout owner so the leading view remains accessible via Field::get().
bytes = static_cast<char*>(Allocator<false> {}.alloc(value.size()));
byte_size = value.size();
memcpy(bytes, value.data(), value.size());
view = StringView(bytes, value.size());
}
OwnedBinaryField(const OwnedBinaryField&) = delete;
OwnedBinaryField& operator=(const OwnedBinaryField&) = delete;

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Allocate this retained payload through Doris's checked allocator. Every non-inline VARBINARY value materialized as a Field now duplicates value.size() bytes with raw new[]; this is reached by ordinary column extraction plus Iceberg defaults and Parquet metadata, and the declared length can be Integer.MAX_VALUE. The current jemalloc hook only routes allocation calls and does not consume/release the task tracker, while raw new[] also skips Allocator::memory_check and its controlled MEM_ALLOC_FAILED path. A large value (and the temporary/destination deep-copy peak) can therefore exceed a query or process limit without Doris charging it or rejecting it at the configured limit. Please use DorisUniqueBufferPtr or an equivalent allocator-backed owner that still fits Field::storage, and cover allocation/destruction under a limited tracker.

@Gabriel39 Gabriel39 Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 221207e and synchronized to #68297 (0a220b4).

Long binary payloads now use Allocator for checked allocation and release. The RAII owner remains standard-layout and fits Field::storage, preserving the leading StringView access. It stores the allocation length separately because Field::get() exposes a mutable view. Inline payloads remain allocation-free.

Added limited-tracker tests for retained/copy accounting, replacement and destruction, oversized allocation rejection, and failed copy/assignment preserving the original value. Both new tests failed with the original new[] implementation. All 40 focused ASAN BE tests pass on each branch.

Also stabilized the failing historical-offset fixture with fixed zones so the large negative offsets and second-precision round trips do not depend on the host's historical Manila/Guam tzdata. This change stays within the existing PR scope.

clang-format 16 and master build hygiene passed. Full clang-tidy remains blocked by pre-existing diagnostics, including the unmatched NOLINTEND in core/types.h. CI has been retriggered on both PRs.

OwnedBinaryField& operator=(OwnedBinaryField&& other) noexcept {
release_bytes();
view = other.view;
bytes = std::exchange(other.bytes, nullptr);
byte_size = std::exchange(other.byte_size, 0);
return *this;
}
~OwnedBinaryField() { release_bytes(); }

private:
void release_bytes() const {
if (bytes != nullptr) {
// Field::get() exposes a mutable view; release the original allocation size.
Allocator<false> {}.free(bytes, byte_size);
}
}
};
static_assert(std::is_standard_layout_v<OwnedBinaryField>);
static_assert(offsetof(OwnedBinaryField, view) == 0);
} // namespace

template <PrimitiveType Type>
void Field::create_concrete(typename PrimitiveTypeTraits<Type>::CppType&& x) {
// In both Field and PODArray, small types may be stored as wider types,
Expand All @@ -99,7 +145,12 @@ void Field::create_concrete(typename PrimitiveTypeTraits<Type>::CppType&& x) {
// we must initialize the entire wide stored type, and not just the
// nominal type.
using StorageType = typename PrimitiveTypeTraits<Type>::CppType;
new (&storage) StorageType(std::move(x));
if constexpr (Type == TYPE_VARBINARY) {
static_assert(sizeof(OwnedBinaryField) <= sizeof(storage));
new (&storage) OwnedBinaryField(x);
} else {
new (&storage) StorageType(std::move(x));
}
type = Type;
DCHECK_NE(type, PrimitiveType::INVALID_TYPE);
}
Expand All @@ -112,7 +163,11 @@ void Field::create_concrete(const typename PrimitiveTypeTraits<Type>::CppType& x
// we must initialize the entire wide stored type, and not just the
// nominal type.
using StorageType = typename PrimitiveTypeTraits<Type>::CppType;
new (&storage) StorageType(x);
if constexpr (Type == TYPE_VARBINARY) {
new (&storage) OwnedBinaryField(x);
} else {
new (&storage) StorageType(x);
}
type = Type;
DCHECK_NE(type, PrimitiveType::INVALID_TYPE);
}
Expand Down Expand Up @@ -241,6 +296,8 @@ Field& Field::operator=(const Field& rhs) {
if (this != &rhs) {
if (type != rhs.type) {
destroy();
// A failed allocation while changing types must leave a destructible Field.
type = TYPE_NULL;
create(rhs);
} else {
assign(rhs); /// This assigns string or vector without deallocation of existing buffer.
Expand Down Expand Up @@ -646,12 +703,20 @@ void Field::assign(const Field& field) {
/// Assuming same types.
template <PrimitiveType Type>
void Field::assign_concrete(typename PrimitiveTypeTraits<Type>::CppType&& x) {
if constexpr (Type == TYPE_VARBINARY) {
*reinterpret_cast<OwnedBinaryField*>(&storage) = OwnedBinaryField(x);
return;
}
auto* MAY_ALIAS ptr = reinterpret_cast<typename PrimitiveTypeTraits<Type>::CppType*>(&storage);
*ptr = std::forward<typename PrimitiveTypeTraits<Type>::CppType>(x);
}

template <PrimitiveType Type>
void Field::assign_concrete(const typename PrimitiveTypeTraits<Type>::CppType& x) {
if constexpr (Type == TYPE_VARBINARY) {
*reinterpret_cast<OwnedBinaryField*>(&storage) = OwnedBinaryField(x);
return;
}
auto* MAY_ALIAS ptr = reinterpret_cast<typename PrimitiveTypeTraits<Type>::CppType*>(&storage);
*ptr = std::forward<const typename PrimitiveTypeTraits<Type>::CppType>(x);
}
Expand Down Expand Up @@ -683,6 +748,10 @@ const typename PrimitiveTypeTraits<T>::CppType& Field::get() const {

template <PrimitiveType T>
void Field::destroy() {
if constexpr (T == TYPE_VARBINARY) {
reinterpret_cast<OwnedBinaryField*>(&storage)->~OwnedBinaryField();
return;
}
using TargetType = typename PrimitiveTypeTraits<T>::CppType;
DCHECK(T == type || ((is_string_type(type) && is_string_type(T))))
<< "Type mismatch: requested " << type_to_string(T) << ", actual " << get_type_name();
Expand Down
7 changes: 5 additions & 2 deletions be/src/core/field.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,13 +191,15 @@ class Field {
Field(PrimitiveType w) : type(w) {}
template <PrimitiveType T>
static Field create_field(const typename PrimitiveTypeTraits<T>::CppType& data) {
auto f = Field(T);
// Publish the type only after construction succeeds, so allocation failures cannot
// destroy uninitialized owned storage (including long binary values).
auto f = Field();
f.template create_concrete<T>(data);
return f;
}
template <PrimitiveType T>
static Field create_field(typename PrimitiveTypeTraits<T>::CppType&& data) {
auto f = Field(T);
auto f = Field();
f.template create_concrete<T>(std::move(data));
return f;
}
Expand Down Expand Up @@ -243,6 +245,7 @@ class Field {
if (this != &rhs) {
if (type != rhs.type) {
destroy();
type = TYPE_NULL;
create(std::move(rhs));
} else {
assign(std::move(rhs));
Expand Down
16 changes: 16 additions & 0 deletions be/src/core/value/timestamptz_value.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#include "core/value/timestamptz_value.h"

#include "common/exception.h"
#include "exprs/function/cast/cast_to_timestamptz_impl.hpp"

namespace doris {
Expand All @@ -38,6 +39,13 @@ std::string TimestampTzValue::to_string(const cctz::time_zone& tz, int scale) co
auto lookup_result = tz.lookup(cur_tz_time);

cctz::civil_second civ = lookup_result.cs;
// UTC storage bounds do not guarantee a representable session-local year. Reject
// overflow before DateTimeV2 formatting could produce an offset-only wire value.
if (civ.year() < 0 || civ.year() > 9999) {

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Keep the remaining TIMESTAMP_NS error paths from re-entering this throwing formatter. With the session zone +08:00, the maximum UTC TIMESTAMPTZ first fails local conversion because the year becomes 10000; strict TIMESTAMPTZ-to-TIMESTAMP_NS cast and TIMESTAMPTZ/TIMESTAMP_NS comparison then call to_string(local_zone) while building their InvalidArgument Status, so this exception escapes before that Status is returned. The parallel cast paths changed in this PR already render utc_dt() instead. Please update these two missed callers the same way and add strict-cast and comparison boundary tests.

@Gabriel39 Gabriel39 Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0765475. Both remaining TIMESTAMP_NS error paths now format the valid UTC value instead of calling the session-local formatter again. ASAN tests cover both year endpoints, strict errors/non-strict NULL results, and both comparison operand orders; all 31 focused BE tests passed.

throw Exception(ErrorCode::INVALID_ARGUMENT,
"TIMESTAMPTZ local year is outside [0, 9999]: year={}, timezone={}",
civ.year(), tz.name());
}
auto time_offset = lookup_result.offset;

bool is_negative_offset = time_offset < 0;
Expand Down Expand Up @@ -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
// instant represented by the client-visible wall clock and offset when read back.
const int offset_seconds = abs_offset % 60;

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Update the parallel to_iso8601(TIMESTAMPTZ) formatter too. Its specialization still emits only +HH:MM (and derives the sign from truncated offset_hours), so a historical offset such as +08:05:43 is rendered as +08:05, denoting a different instant; it also unchecked-casts the session-local year instead of applying this new boundary guard. Please share this formatter or mirror both fixes, increase its 32-byte maximum for :SS, and add historical-offset and boundary coverage.

@Gabriel39 Gabriel39 Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The separate to_iso8601 implementation and its historical-offset/year-boundary limitations predate this PR. It is unchanged here; repairing that formatter would broaden the follow-up beyond newly introduced defects.

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Keep MySQL column metadata wide enough for the new format. With scale 6, appending :SS produces a 35-byte value (YYYY-MM-DD HH:MM:SS.ffffff+HH:MM:SS), and both text and prepared-result paths send that full string. MysqlSerializer.getMysqlTypeLength(TIMESTAMPTZ), however, still advertises 32 bytes based on +HH:mm, so clients see a display width smaller than rows this formatter now emits. Please raise the TIMESTAMPTZ metadata width to 35 and cover the serialized Column Definition value.

Comment thread
yiguolei marked this conversation as resolved.
if (offset_seconds != 0) {
buffer[len++] = ':';
buffer[len++] = static_cast<char>('0' + offset_seconds / 10);
buffer[len++] = static_cast<char>('0' + offset_seconds % 10);
}
return {buffer, static_cast<size_t>(len)};
}

Expand Down
7 changes: 7 additions & 0 deletions be/src/exec/common/hash_table/hash_key_type.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

@github-actions github-actions Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Preserve the existing serialized path for supported multi-key consumers. This pre-check now runs before the multi-key branch, so it also rejects keys that previously selected HashKeyType::serialized: default-enabled PartitionTopN windows, multi-column INTERSECT/EXCEPT, and recursive UNION DISTINCT. Their PartitionedHashMapVariants, SetDataVariants, and DistinctDataVariants all implement serialized keys, and ColumnVarbinary supplies serialization; only single-key VARBINARY was already unsupported. Please move the rejection to the join/group consumers that require it (or otherwise retain these byte-safe consumers) and add multi-key window, set-operation, and recursive-CTE coverage.

@Gabriel39 Gabriel39 Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rejection is intentional for this change. Preserving previously reachable serialized hash consumers is a compatibility concern, so the common VARBINARY hash restriction is retained.

throw Exception(ErrorCode::NOT_IMPLEMENTED_ERROR,
"VARBINARY hash keys are not supported");
}
}
if (data_types.size() > 1) {
return get_hash_key_type_fixed(data_types);
}
Expand Down
Loading
Loading