github-actions[bot] commented on code in PR #68297:
URL: https://github.com/apache/doris/pull/68297#discussion_r4067965085
##########
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
+ // instant represented by the client-visible wall clock and offset when
read back.
+ const int offset_seconds = abs_offset % 60;
Review Comment:
[P2] Apply the same offset and year guarantees to `to_iso8601`
The separately registered `to_iso8601(TIMESTAMPTZ)` specialization in
`date_time_transforms.h` does not use this corrected formatter: it still
appends only `+/-HH:MM` and performs no local-year check. Thus a historical
zone such as `Asia/Shanghai` loses its 43 offset seconds (the rendered ISO
value denotes a different instant), while a maximum UTC value under `+08:00`
can reach local year 10000 and degrade to an offset-only result. Please share
this checked formatting path (using `T` as the separator), increase that
function's 32-byte capacity for `:SS`, and cover both cases.
##########
be/src/core/field.cpp:
##########
@@ -90,6 +92,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;
+ 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.
Review Comment:
[P1] Keep mutable VARBINARY access synchronized with the owner
`Field::get<TYPE_VARBINARY>()` still returns a mutable `StringView&` to this
leading member, so callers can now bypass the new owner. The added test assigns
a 12-byte inline value and happens to remain safe, but assigning a longer view
stores only the caller's pointer while `bytes`/`byte_size` still describe the
old allocation; once that source buffer dies, subsequent reads use a dangling
pointer. Likewise, shrinking an external view through `set_size()` to 12 bytes
flips `data()` to the incomplete inline representation. Please make this
specialization const-only or route mutation through an owner-aware setter, and
cover a non-inline local buffer whose lifetime ends plus the 12/13-byte
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]
