morningman commented on code in PR #69771:
URL: https://github.com/apache/doris/pull/69771#discussion_r4407181913
##########
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:
Fixed in 3b3f2a6dec6. The mechanism is right, and it is older than this PR:
nothing has called `ResultQueueMgr::update_queue_status` since the non-pipeline
engine was removed (#35803), so any failure on this path (a scan error, an
expression error, a cancellation such as a memory-limit kill) already ended a
`_query_plan` read as a complete result; the rejected batch is one more such
failure. `MemoryScratchSinkLocalState::close` now records a non-OK
`exec_status` in the queue before it puts the sentinel, and
`ResultQueueMgr::fetch_result` checks the queue status again when it takes the
sentinel, which covers a fetch already blocked in `blocking_get`.
Tests: BE UT
`ResultQueueMgrTest.fetch_result_reports_failure_recorded_before_sentinel` (a
fetch waiting for data) and `fetch_result_end_after_ok_status`; regression
`arrow_flight_sql_p0/test_query_plan_scan_validation` reads `_query_plan` scans
through `open_scanner` / `get_next`. On the previous head its invalid UTF-8
scan read 0 rows and ended with eos; now `get_next` returns
`[INVALID_ARGUMENT]Invalid Arrow Flight result in column 2 (''): Invalid:
Invalid UTF8 sequence at string index 1`, the year-0 scan returns the timestamp
error, and a valid scan still reads all rows. branch-4.1 / 4.2 have the same
`close()` and already ship this validation; that is passed on to their
maintainers.
##########
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:
Fixed in 3489284b381. Agreed: pytz rounds every offset with `(offset + 30)
// 60 * 60`, and PyArrow uses it for zone names when it is installed. The zoned
bounds are now computed with both the exact and the pytz-rounded offsets at
0001-01-01 and 10000-01-01, and the stricter one wins; for America/Chicago the
lower bound moves from 05:50:36Z to 05:51:00Z. Whole-minute offsets (UTC, fixed
offsets, the year-9999 offsets) keep their bounds. For a zone whose local mean
time rounds to a more negative offset, up to 30 seconds at the start of year 1
are now also rejected for zoneinfo clients; one bound that holds for both
decoders is the trade-off we take. New UT
`ArrowFlightTimestampTest.ZonedLowerBoundHoldsWithPytzRoundedOffsets`: the
exact-offset start is rejected and 05:51:00Z is accepted; it fails when the
bound uses the exact offset.
--
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]
