github-actions[bot] commented on code in PR #65689:
URL: https://github.com/apache/doris/pull/65689#discussion_r3611975754
##########
be/src/core/data_type_serde/data_type_datetimev2_serde.cpp:
##########
@@ -525,16 +525,24 @@ Status
DataTypeDateTimeV2SerDe::read_column_from_arrow(IColumn& column,
for (auto value_i = start; value_i < end; ++value_i) {
const uint8_t* raw_byte_ptr = base_ptr + value_i * element_size;
auto date_value = unaligned_load<int64_t>(raw_byte_ptr);
- auto utc_epoch = static_cast<UInt64>(date_value);
DateV2Value<DateTimeV2ValueType> v;
- // convert second
- v.from_unixtime(utc_epoch / divisor, ctz);
- // get rest time
+ // C++ integer division truncates toward zero. Normalize the
remainder so a negative
+ // timestamp still has a non-negative fractional part, e.g.
-876544us becomes
+ // -1 second and 123456us.
+ int64_t seconds = date_value / divisor;
+ int64_t remainder = date_value % divisor;
+ if (remainder < 0) {
+ --seconds;
+ remainder += divisor;
+ }
+ v.from_unixtime(seconds, ctz);
Review Comment:
[P1] Please reject semantically non-null timestamps outside DateTimeV2's
civil range before this unchecked conversion. For example, UTC `timestamp[us] =
-62167219201000000` normalizes to civil year -1; `from_unixtime` narrows that
to `uint16_t` year 65535 (above `MAX_YEAR = 9999`), the value is appended, and
this reader still returns OK. Year 10000 has the same problem at the upper
bound, and callers perform no later validation. Please validate the
post-timezone civil fields before narrowing/packing and return non-OK for
invalid live values, while preserving Arrow null semantics at every enclosing
nullable/complex level so masked payloads cannot fail conversion. Cover both
bounds plus a masked out-of-range payload in tests.
##########
be/src/core/data_type_serde/data_type_datetimev2_serde.cpp:
##########
@@ -525,16 +525,24 @@ Status
DataTypeDateTimeV2SerDe::read_column_from_arrow(IColumn& column,
for (auto value_i = start; value_i < end; ++value_i) {
const uint8_t* raw_byte_ptr = base_ptr + value_i * element_size;
auto date_value = unaligned_load<int64_t>(raw_byte_ptr);
- auto utc_epoch = static_cast<UInt64>(date_value);
DateV2Value<DateTimeV2ValueType> v;
- // convert second
- v.from_unixtime(utc_epoch / divisor, ctz);
- // get rest time
+ // C++ integer division truncates toward zero. Normalize the
remainder so a negative
+ // timestamp still has a non-negative fractional part, e.g.
-876544us becomes
+ // -1 second and 123456us.
+ int64_t seconds = date_value / divisor;
+ int64_t remainder = date_value % divisor;
+ if (remainder < 0) {
+ --seconds;
+ remainder += divisor;
+ }
+ v.from_unixtime(seconds, ctz);
+ // Get the fractional part.
// add 0 on the right to make it 6 digits. DateTimeV2Value
microsecond is 6 digits,
// the scale decides to keep the first few digits, so the valid
digits should be kept at the front.
- // "2022-01-01 11:11:11.111", utc_epoch = 1641035471111, divisor =
1000, set_microsecond(111000)
- v.set_microsecond((utc_epoch % divisor) * DIVISOR_FOR_MICRO /
divisor);
+ // "2022-01-01 11:11:11.111", timestamp = 1641035471111, divisor =
1000,
+ // set_microsecond(111000)
+ v.set_microsecond(remainder * DIVISOR_FOR_MICRO / divisor);
Review Comment:
[P1] Please apply the destination `_scale` before storing each semantically
non-null fraction. A `DataTypeDateTimeV2SerDe(3)` reading raw MICRO `-876444`
currently packs `.123556`, even though the established DateTimeV2
scale-conversion path rounds that value to `.124000`. Scale-aware formatting
can hide the extra digits, but packed comparisons and persistence still retain
them, and no caller requires the Arrow unit to match the slot scale. Please
reuse the established scale semantics (including second carry/error handling),
or reject finer units; masked payloads must remain non-observable. Add raw
packed-microsecond and carry tests.
--
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]
