github-actions[bot] commented on code in PR #65674: URL: https://github.com/apache/doris/pull/65674#discussion_r3615539608
########## be/src/core/data_type_serde/parquet_timestamp.h: ########## @@ -0,0 +1,90 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#pragma once + +#include <cstdint> +#include <limits> + +#include "common/status.h" +#include "core/data_type_serde/parquet_decode_source.h" + +namespace doris { + +#pragma pack(1) +struct ParquetInt96Timestamp { + int64_t nanos_of_day; + int32_t julian_day; +}; +#pragma pack() +static_assert(sizeof(ParquetInt96Timestamp) == 12); + +inline constexpr int64_t MIN_DORIS_TIMESTAMP_MICROS = -62135596800000000LL; +inline constexpr int64_t MAX_DORIS_TIMESTAMP_MICROS = 253402300799999999LL; + +inline Status validate_parquet_timestamp_micros(int64_t timestamp_micros) { + if (timestamp_micros < MIN_DORIS_TIMESTAMP_MICROS || + timestamp_micros > MAX_DORIS_TIMESTAMP_MICROS) { + return Status::DataQualityError( + "Parquet timestamp is outside the Doris 0001-9999 range: micros={}", + timestamp_micros); + } + return Status::OK(); +} + +inline Status parquet_timestamp_micros(ParquetTimeUnit unit, int64_t value, int64_t* result) { + if (unit == ParquetTimeUnit::MILLIS) { + // Validate in the source unit before scaling; signed overflow could otherwise turn an + // invalid file value into an in-range timestamp that survives later range checks. + if (value > std::numeric_limits<int64_t>::max() / 1000 || + value < std::numeric_limits<int64_t>::min() / 1000) { + return Status::DataQualityError("Parquet timestamp overflows microseconds"); + } + *result = value * 1000; + } else if (unit == ParquetTimeUnit::NANOS) { + *result = value / 1000; Review Comment: [P1] Floor negative nanoseconds when reducing timestamp precision. Signed C++ division makes `-1` ns become `0` us and `-1001` ns become `-1` us, so valid pre-epoch Parquet timestamps materialize one microsecond late; the later epoch-microsecond conversion already normalizes negative remainders into the preceding second. Apply the same floor-division rule here, and cover nonmultiples such as `-1`, `-999`, and `-1001` through plain and dictionary DATETIMEV2/TIMESTAMPTZ reads. ########## be/src/format_v2/parquet/parquet_scan.cpp: ########## @@ -1303,44 +1760,68 @@ Status ParquetScanScheduler::read_filter_columns(int64_t batch_rows, // Single-column conjuncts can be evaluated immediately after their column is read. Once // selection shrinks, later predicate columns use ParquetColumnReader::select() so the // reader skips rows already rejected by earlier predicates instead of materializing them. - for (size_t idx = 0; idx < request.predicate_columns.size(); ++idx) { + _ordered_predicate_positions_scratch = detail::order_adaptive_predicates( Review Comment: [P1] Preserve data-dependent errors before adaptively reordering predicates. This reorder assumes every scheduled conjunct may skip rows rejected by an earlier predicate, but the schedule's sole gate is `is_safe_to_execute_on_selected_rows()`, and `VectorizedFnCall` only blacklists `assert_true`; deterministic `mod` is admitted even though `mod(MIN_SIGNED, -1)` raises `INVALID_ARGUMENT`. A request that originally evaluates `mod` first can therefore stop raising after warmup moves a selective predicate ahead of it and removes the failing row. Require a total-function guarantee before allowing selected-row execution/reordering (or mark every throwing function unsafe), and add a two-batch regression with the failing row rejected by the newly first predicate. -- 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]
