github-actions[bot] commented on code in PR #65823:
URL: https://github.com/apache/doris/pull/65823#discussion_r3694597968
##########
be/src/core/data_type_serde/data_type_datetimev2_serde.cpp:
##########
@@ -591,57 +553,41 @@ Status
DataTypeDateTimeV2SerDe::read_column_from_arrow(IColumn& column,
check_arrow_no_offset(*arrow_array);
}
auto& col_data = static_cast<ColumnDateTimeV2&>(column).get_data();
- int64_t divisor = 1;
if (arrow_array->type()->id() == arrow::Type::TIMESTAMP) {
const auto* concrete_array = dynamic_cast<const
arrow::TimestampArray*>(arrow_array);
const auto type =
std::static_pointer_cast<arrow::TimestampType>(arrow_array->type());
- switch (type->unit()) {
- case arrow::TimeUnit::type::SECOND: {
- divisor = DIVISOR_FOR_SECOND;
- break;
- }
- case arrow::TimeUnit::type::MILLI: {
- divisor = DIVISOR_FOR_MILLI;
- break;
- }
- case arrow::TimeUnit::type::MICRO: {
- divisor = DIVISOR_FOR_MICRO;
- break;
- }
- case arrow::TimeUnit::type::NANO: {
- divisor = DIVISOR_FOR_NANO;
- break;
- }
- default: {
- LOG(WARNING) << "not support convert to datetimev2 from
time_unit:" << type->unit();
- return Status::InvalidArgument("not support convert to datetimev2
from time_unit: {}",
- type->unit());
- }
- }
const auto* base_ptr = reinterpret_cast<const
uint8_t*>(concrete_array->raw_values());
const size_t element_size = sizeof(int64_t);
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);
-
- DateV2Value<DateTimeV2ValueType> v;
- // 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;
+ int64_t timestamp_micros;
+ switch (type->unit()) {
+ case arrow::TimeUnit::type::SECOND:
+ timestamp_micros = date_value * 1000000;
Review Comment:
[P1] Range-check before scaling Arrow seconds and milliseconds
Arrow timestamp storage is a signed 64-bit count, so this input can be
outside Doris' range while still being a valid `TimestampArray`. For example,
`INT64_MAX * 1000000` invokes signed-overflow undefined behavior before
`append_datetimev2_from_utc_epoch_micros` can validate it; on ordinary
two's-complement wrapping it becomes `-1000000` and is silently accepted as
`1969-12-31 23:59:59`. The MILLI branch has the same problem, and Arrow stream,
Remote Doris, and Paimon readers all reach this code. Please bound-check in the
source unit or scale in widened arithmetic before narrowing, then validate the
Doris range, with `INT64_MIN`/`INT64_MAX` and boundary tests.
##########
be/src/core/data_type_serde/data_type_datetimev2_serde.cpp:
##########
@@ -558,16 +517,19 @@ Status
DataTypeDateTimeV2SerDe::write_column_to_arrow(const IColumn& column,
auto& timestamp_builder =
assert_cast<arrow::TimestampBuilder&>(*array_builder);
std::shared_ptr<arrow::TimestampType> timestamp_type =
std::static_pointer_cast<arrow::TimestampType>(array_builder->type());
- const std::string& timezone = timestamp_type->timezone();
- const cctz::time_zone& real_ctz = timezone.empty() ? cctz::utc_time_zone()
: ctz;
+ static const auto utc_timezone = cctz::utc_time_zone();
+ // DATETIMEV2 is normally written as a timezone-naive Arrow timestamp.
Keep support for
+ // timezone-aware target schemas because external tables may expose a
timestamp-with-timezone
+ // column as DATETIMEV2 when timezone type mapping is disabled.
+ const auto& conversion_timezone = timestamp_type->timezone().empty() ?
utc_timezone : ctz;
Review Comment:
[P1] Preserve naive DATETIMEV2 through Hive's forced-INT96 boundary
Hive forces Parquet timestamps to INT96, which has no logical timezone
marker. With an empty-timezone Arrow schema, this line encodes an Asia/Shanghai
wall-clock `12:00` as the epoch count for `12:00` UTC; after INT96 drops the
marker, both Doris INT96 DATETIMEV2 readers apply the session timezone and
reconstruct `20:00`. This breaks a same-version Hive insert/read round trip in
every non-UTC session (the pre-change aware path encoded through `ctz` and
returned `12:00`). Please keep the aware/common-denominator encoding for forced
INT96 or otherwise preserve local-timestamp semantics across that boundary, and
add a non-UTC Hive DATETIMEV2 round-trip test.
##########
be/src/core/data_type_serde/data_type_datetimev2_serde.cpp:
##########
@@ -591,57 +553,41 @@ Status
DataTypeDateTimeV2SerDe::read_column_from_arrow(IColumn& column,
check_arrow_no_offset(*arrow_array);
}
auto& col_data = static_cast<ColumnDateTimeV2&>(column).get_data();
- int64_t divisor = 1;
if (arrow_array->type()->id() == arrow::Type::TIMESTAMP) {
const auto* concrete_array = dynamic_cast<const
arrow::TimestampArray*>(arrow_array);
const auto type =
std::static_pointer_cast<arrow::TimestampType>(arrow_array->type());
- switch (type->unit()) {
- case arrow::TimeUnit::type::SECOND: {
- divisor = DIVISOR_FOR_SECOND;
- break;
- }
- case arrow::TimeUnit::type::MILLI: {
- divisor = DIVISOR_FOR_MILLI;
- break;
- }
- case arrow::TimeUnit::type::MICRO: {
- divisor = DIVISOR_FOR_MICRO;
- break;
- }
- case arrow::TimeUnit::type::NANO: {
- divisor = DIVISOR_FOR_NANO;
- break;
- }
- default: {
- LOG(WARNING) << "not support convert to datetimev2 from
time_unit:" << type->unit();
- return Status::InvalidArgument("not support convert to datetimev2
from time_unit: {}",
- type->unit());
- }
- }
const auto* base_ptr = reinterpret_cast<const
uint8_t*>(concrete_array->raw_values());
const size_t element_size = sizeof(int64_t);
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);
Review Comment:
[P1] Do not decode the value payload of null Arrow timestamps
`DataTypeNullableSerDe` records the validity bitmap and then delegates the
whole array to this nested reader. Arrow gives no semantic meaning to a null
slot's value bytes, but this loop still scales and range-checks them. A valid
nullable SECOND array with a null slot containing `253402300800` therefore
fails with a year-10000 `DataQualityError` instead of producing NULL; external
Arrow/Remote Doris/Paimon producers need not zero those bytes. Please detect
null slots here and append a default nested value without decoding the payload,
and add a mixed null/non-null test with a poisoned raw value.
##########
be/src/format/arrow/arrow_row_batch.cpp:
##########
@@ -97,17 +97,25 @@ Status convert_to_arrow_type(const DataTypePtr& origin_type,
case TYPE_DATEV2:
*result = std::make_shared<arrow::Date32Type>();
break;
- // TODO: maybe need to distinguish TYPE_DATETIME and TYPE_TIMESTAMPTZ
case TYPE_TIMESTAMPTZ:
- case TYPE_DATETIMEV2:
+ case TYPE_DATETIMEV2: {
+ arrow::TimeUnit::type time_unit;
if (type->get_scale() > 3) {
- *result =
std::make_shared<arrow::TimestampType>(arrow::TimeUnit::MICRO, timezone);
+ time_unit = arrow::TimeUnit::MICRO;
} else if (type->get_scale() > 0) {
- *result =
std::make_shared<arrow::TimestampType>(arrow::TimeUnit::MILLI, timezone);
+ time_unit = arrow::TimeUnit::MILLI;
} else {
- *result =
std::make_shared<arrow::TimestampType>(arrow::TimeUnit::SECOND, timezone);
+ time_unit = arrow::TimeUnit::SECOND;
+ }
+ // Doris DATETIMEV2 is a wall-clock value without a timezone, while
TIMESTAMPTZ represents
+ // an instant with timezone semantics.
+ if (type->get_primitive_type() == TYPE_DATETIMEV2) {
+ *result = std::make_shared<arrow::TimestampType>(time_unit);
Review Comment:
[P1] Version the global DATETIMEV2 Arrow contract
This helper is shared beyond the Flight result path covered by the existing
schema-equality thread. A new Remote Doris producer now encodes wall-clock
`12:00` as a naive UTC count, but an old consumer still applies its local `ctz`
unconditionally, so an Asia/Shanghai reader silently returns `20:00`. The same
helper feeds ordinary Parquet OUTFILE schemas; during a mixed-BE parallel
OUTFILE, old writers emit adjusted-to-UTC timestamps while new writers emit
non-adjusted timestamps and different physical counts, leaving one data set
with incompatible files. Please negotiate/pass a query or protocol capability
(or retain the aware common denominator until every participant supports the
new contract), and cover old-reader/new-producer Remote Doris plus
mixed-version Parquet writers. This is distinct from the existing thread, which
only covers same-query Flight `Schema.equals` failure.
--
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]
