github-actions[bot] commented on code in PR #69771:
URL: https://github.com/apache/doris/pull/69771#discussion_r4400505772
##########
be/src/format/arrow/arrow_block_convertor.cpp:
##########
@@ -62,6 +65,134 @@ namespace doris {
namespace {
+class FlightTimestampValidator {
+public:
+ Status init(std::shared_ptr<arrow::Array> array, std::string path,
+ const cctz::time_zone& timezone) {
+ _array = std::move(array);
+ _path = std::move(path);
+ switch (_array->type_id()) {
+ case arrow::Type::TIMESTAMP: {
+ const auto& type = static_cast<const
arrow::TimestampType&>(*_array->type());
+ int64_t units_per_second = 1;
+ switch (type.unit()) {
+ case arrow::TimeUnit::SECOND:
+ units_per_second = 1;
+ break;
+ case arrow::TimeUnit::MILLI:
+ units_per_second = 1000;
+ break;
+ case arrow::TimeUnit::MICRO:
+ units_per_second = 1000000;
+ break;
+ case arrow::TimeUnit::NANO:
+ units_per_second = 1000000000;
+ break;
+ }
+ int64_t min_seconds = -62135596800LL;
+ int64_t end_seconds = 253402300800LL;
+ if (!type.timezone().empty()) {
+ // The ordinary Arrow writer has already checked the
schema/timezone binding.
+ const auto local_start = cctz::convert(cctz::civil_second(1,
1, 1), timezone);
+ const auto local_end = cctz::convert(cctz::civil_second(10000,
1, 1), timezone);
+ // Python first constructs UTC and then applies the Arrow
timezone. Both calendar
+ // representations must fit, including offsets at the first
and last supported day.
+ min_seconds =
Review Comment:
[P2] Account for PyArrow's historical timezone offsets at the lower bound
For `time_zone='America/Chicago'`, a stored TIMESTAMPTZ(6) value `0001-01-01
05:50:36.000000 +00:00` produces `-62135575764000000` us. The [TZDB
offset](https://github.com/eggert/tz/blob/main/northamerica#L401) is -05:50:36,
so this equals cctz's `local_start` and passes validation. However, [PyArrow
chooses pytz when
installed](https://github.com/apache/arrow/blob/apache-arrow-24.0.0/python/pyarrow/src/arrow/python/datetime.cc#L341-L374),
and [pytz rounds historical offsets to
minutes](https://github.com/stub42/pytz/blob/master/src/pytz/tzfile.py#L103-L109),
giving -05:51 here. The inferred Python local datetime is then 24 seconds
before year 1: [TimestampScalar.as_py() converts UTC using
astimezone](https://github.com/apache/arrow/blob/apache-arrow-24.0.0/python/pyarrow/scalar.pxi#L656-L679)
and still raises `OverflowError` after this batch is published. The
fixed-offset tests do not cover this named-zone case. Account for the supported
decoder's timezone rules in the safe bound
and add a named-zone boundary case; the zoneinfo path without pytz preserves
seconds and is unaffected.
##########
be/src/format/arrow/arrow_block_convertor.cpp:
##########
@@ -324,6 +455,31 @@ Status ArrowFlightArrowBlockConvertor::write_column(const
std::shared_ptr<const
array_builder, start, end,
ctz);
}
+Status ArrowFlightArrowBlockConvertor::convert_to_arrow(const Block& block,
arrow::MemoryPool* pool,
+
std::shared_ptr<arrow::RecordBatch>* result,
+ size_t start_row,
size_t end_row) const {
+ std::shared_ptr<arrow::RecordBatch> batch;
+ RETURN_IF_ERROR(ArrowBlockConvertor::convert_to_arrow(block, pool, &batch,
start_row, end_row));
+ // String builders accept arbitrary bytes, but Flight UTF-8 values must be
valid per row,
+ // including nested children. Validate before publishing the batch to the
reader.
+ for (int i = 0; i < batch->num_columns(); ++i) {
+ auto status = batch->column(i)->ValidateFull();
+ if (!status.ok()) {
+ return Status::InvalidArgument("Invalid Arrow Flight result in
column {} ('{}'): {}",
+ i + 1,
batch->schema()->field(i)->name(),
+ status.ToString());
+ }
+ // Arrow accepts the full int64 timestamp domain; ValidateFull cannot
enforce the calendar
+ // range required by Flight clients. Check the encoded values before
publishing the batch.
+ FlightTimestampValidator validator;
+ RETURN_IF_ERROR(
+ validator.init(batch->column(i),
batch->schema()->field(i)->name(), _timezone));
+ RETURN_IF_ERROR(validator.validate(0, batch->num_rows(), start_row));
Review Comment:
[P1] Propagate validation failures to external scan readers
`MemoryScratchSinkOperatorX::sink_impl()` also calls this converter for
Spark/Flink `_query_plan` scans. With a stored year-zero DATETIMEV2 (or invalid
UTF-8 STRING), the new validation error returns before that batch is queued.
The pipeline then closes the sink, but `MemoryScratchSinkLocalState::close()`
ignores `exec_status` and queues `nullptr` without updating the queue's status.
`ResultQueueMgr::fetch_result()` interprets that sentinel as successful EOF,
and `BaseBackendService::get_next()` has no separate query-status check;
external fragments also skip FE status reporting. Consequently these scans now
succeed with zero or partial rows, silently losing the rejected batch and
remaining data. Propagate the failure through the external scanner's
queue/get_next path, including a fetch already waiting for data, and add a
scanner-level regression before enabling this rejection for that consumer.
--
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]
