morningman commented on code in PR #69697:
URL: https://github.com/apache/doris/pull/69697#discussion_r4408867715
##########
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:
Fixed in 6c1fd8f8ab0. The range check before packing stays, and
`append_timestamptz_from_utc_epoch_micros` checks `is_valid_date()` at run time
again after packing instead of the DCHECK, so -62162121600000000 (0000-02-29
UTC) fails with `Decoded TIMESTAMPTZ is not a valid Doris date`, as it did on
master. The DATETIMEV2 Arrow path checked only the civil year and stored that
date as an invalid DATETIMEV2 (it did so before this PR too); it now checks
`is_valid_date()` after packing as well.
`RejectsArrowTimestampsOutsideTimestampTzRange` and
`RejectsArrowTimestampsOutsideDateTimeV2Range` read the leap day and expect
DataQualityError; both fail when the new checks skip that value.
`orc_timestamp_to_datetime()` has the same year-only check and is left to a
change of its own.
##########
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:
Agreed on the mechanism. The fix is the NULL-parent masking in #69581 rather
than a second copy here: df95b41a331
(`mask_arrow_children_hidden_by_null_parents`) masks the child slots of NULL
list, map and struct rows in the shared nested SerDes before any leaf SerDe
reads them, so the leaf checks of this PR never see a hidden payload, for every
Arrow caller. `ArrowParentNullsTest` there covers mixed NULL / non-NULL parents
and child offsets.
I checked your example on a tree with both PRs: a STRUCT with rows `[NULL,
{ts: 0}]` whose NULL row carries 2005980768000000000, and a LIST `[NULL, [0]]`
whose NULL row spans that value, both read two rows with row 0 NULL. With the
masking taken out, the STRUCT case fails with `Arrow timestamp
2005980768000000000 of type timestamp[us] is outside the DATETIMEV2 range`.
This PR will be merged after #69581, so master does not carry the regression in
between; the description now says so. When this PR is rebased onto #69581, I
will add your STRUCT and LIST cases here as a 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]
