github-actions[bot] commented on code in PR #66740:
URL: https://github.com/apache/doris/pull/66740#discussion_r3775499285
##########
be/src/core/value/timestamptz_value.cpp:
##########
@@ -65,6 +66,11 @@ 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;
+ if (offset_secs != 0) {
Review Comment:
[P1] Update the parallel `to_iso8601(TIMESTAMPTZ)` formatter too.
`ToIso8601Impl<TYPE_TIMESTAMPTZ>` in `date_time_transforms.h` reads the same
cctz offset but always writes only `HH:MM`, so this Shanghai value is still
rendered as `1900-01-01T08:05:43.000000+08:05` (an instant 43 seconds late).
That implementation also chooses the sign from `time_offset / 3600`, making
`-00:30` and `-00:30:45` positive. Please share the corrected offset formatting
with this path, adjust its size contract, and add historical positive/negative
ISO tests.
##########
be/test/core/data_type/data_type_timestamptz_test.cpp:
##########
@@ -113,6 +113,24 @@ TEST_F(DataTypeTimeStampTzTest,
test_to_string_negative_sub_hour_offset) {
EXPECT_EQ(value.to_string(time_zone), "2023-12-31 23:45:00.000000-00:30");
}
+TEST_F(DataTypeTimeStampTzTest, test_to_string_second_offset_round_trip) {
+ TimestampTzValue value = make_timestamptz(1900, 1, 1, 0, 0, 0, 0);
+
+ cctz::time_zone shanghai;
+ ASSERT_TRUE(cctz::load_time_zone("Asia/Shanghai", &shanghai));
+ auto rendered = value.to_string(shanghai, 0);
+ EXPECT_EQ(rendered, "1900-01-01 08:05:43+08:05:43");
+
+ DataTypeSerDe::FormatOptions options;
+ options.timezone = &shanghai;
+ auto column = type->create_column();
+ ASSERT_TRUE(serder->from_string(StringRef {rendered}, *column,
options).ok());
Review Comment:
[P1] Make this test call compilable before relying on its round-trip
assertion. `serder` is a `DataTypeSerDeSPtr`, and both the virtual API and
TIMESTAMPTZ override take the first argument as non-const `StringRef&`; the
temporary `StringRef {rendered}` cannot bind to it. The parser also still needs
the offset-seconds fix noted separately before the assertion can pass.
```suggestion
StringRef rendered_ref {rendered};
ASSERT_TRUE(serder->from_string(rendered_ref, *column, options).ok());
```
##########
be/src/core/value/timestamptz_value.cpp:
##########
@@ -65,6 +66,11 @@ 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;
+ if (offset_secs != 0) {
Review Comment:
[P1] Teach the TIMESTAMPTZ readers about the new offset syntax. This branch
now emits values such as `+08:05:43`, but both parser implementations still
accept only hour/minute offsets: the first rejects Shanghai's minute `05`, the
fallback has the same restriction, and neither consumes the trailing `:43`.
Consequently the added round trip still fails after its call site is made
compilable, and production JSON/text output can emit a TIMESTAMPTZ that its
paired reader cannot ingest. Please extend the strict and fallback parsers plus
fixed-offset construction to preserve optional seconds, and cover
positive/negative strict, non-strict, and JSON round trips.
--
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]
