github-actions[bot] commented on code in PR #69697:
URL: https://github.com/apache/doris/pull/69697#discussion_r4390157310
##########
be/src/core/data_type_serde/data_type_datetimev2_serde.cpp:
##########
@@ -723,7 +723,22 @@ Status
DataTypeDateTimeV2SerDe::read_column_from_arrow(IColumn& column,
--seconds;
remainder += divisor;
}
- v.from_unixtime(seconds, real_ctz);
+ // from_unixtime() narrows the cctz year to uint16_t without a
check, so an instant
+ // whose civil year in real_ctz is outside 0000..9999 would read
as another value:
+ // timestamp(us) 2005980768000000000 (65537-01-01) as 0001-01-01,
and the years
+ // 10000..65535 as dates DATETIMEV2 cannot hold. Check the civil
year before packing,
+ // as orc_timestamp_to_datetime() does for ORC.
+ const auto civil = cctz::convert(
+ cctz::time_point<cctz::seconds>(cctz::seconds(seconds)),
real_ctz);
+ if (civil.year() < 0 || civil.year() > 9999) {
Review Comment:
[P2] Mask NULL ancestors before validating nested timestamp leaves
A valid nullable Arrow STRUCT can have parent validity `[false, true]` and a
non-null `timestamp(MICRO)` child containing `[2005980768000000000, 0]`; its
logical rows are `[NULL, {ts:1970-01-01}]`. [Arrow specifies independent
parent/child
validity](https://arrow.apache.org/docs/format/Columnar.html#struct-validity).
`DataTypeNullableSerDe` copies the parent bitmap, then `DataTypeStructSerDe`
passes `field(i)` unchanged, so the leaf's `IsNull(0)` is false and this new
check rejects the hidden year-65537 payload. The previous conversion's wrapped
year was hidden by the NULL parent, so this batch succeeded. The same
regression affects the new TIMESTAMPTZ check and NULL LIST parents with
arbitrary nonempty child spans. Normalization leaves ordinary struct/list
timestamp arrays unchanged, and stream-load/ADBC callers propagate the error,
failing a read whose visible timestamps are valid. Carry ancestor validity or
skip/default children of NULL parents before applying either check, and
add mixed NULL/non-NULL parent coverage.
##########
be/src/core/data_type_serde/data_type_timestamptz_serde.cpp:
##########
@@ -90,11 +103,7 @@ Status
append_timestamptz_from_utc_epoch_micros(ColumnTimeStampTz::Container& da
TimestampTzValue timestamp_tz;
timestamp_tz.from_unixtime(epoch_seconds, UTC);
timestamp_tz.set_microsecond(static_cast<uint32_t>(micros_of_second));
- if (!timestamp_tz.is_valid_date()) {
- return Status::DataQualityError(
- "Decoded TIMESTAMPTZ is outside the Doris 0001-9999 range:
micros={}",
- timestamp_micros);
- }
+ DCHECK(timestamp_tz.is_valid_date());
Review Comment:
[P2] Keep the runtime calendar-validity check for TIMESTAMPTZ
The new epoch interval does not imply `is_valid_date()`. Arrow
`timestamp(MICRO)` value `-62162121600000000` is UTC `0000-02-29 00:00:00` and
passes these bounds. [cctz's Gregorian leap-year
rule](https://github.com/google/cctz/blob/v2.5/include/cctz/civil_time_detail.h#L78-L80)
accepts year-zero February 29, whereas Doris `is_leap(0)` in `util/time_lut.h`
explicitly returns false, so the resulting value fails
`DateV2Value::is_valid_date()`. Previously this input returned
`DataQualityError`; now it aborts a DCHECK-enabled BE or appends an invalid
TIMESTAMPTZ when DCHECK is disabled (UTC formatting can then emit only
`+00:00`). Keep the new pre-narrowing bounds check, retain a runtime validity
failure after conversion, and add this leap-day input to the unit test.
--
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]
