github-actions[bot] commented on code in PR #65500:
URL: https://github.com/apache/doris/pull/65500#discussion_r3566317107
##########
be/src/format_v2/table_reader.h:
##########
@@ -423,12 +424,20 @@ class TableReader {
DORIS_CHECK(can_filter_all != nullptr);
*can_filter_all = false;
for (const auto& table_filter : _table_filters) {
- if (table_filter.conjunct == nullptr ||
- // RuntimeFilterExpr does not implement execute_column_impl();
it is evaluated by
- // the row-level filter path through execute_filter().
Constant split pruning uses
- // VExprContext::execute() on a one-row synthetic block, so
runtime filters must not
- // be pre-executed here even when their referenced slot maps
to a constant value.
- table_filter.conjunct->root()->is_rf_wrapper() ||
+ if (table_filter.conjunct == nullptr) {
+ continue;
+ }
+ // Constant pruning must preserve a safe prefix of the row-level
conjunct order. Once
+ // an unsafe predicate is reached, evaluating any later constant
predicate could hide
+ // its error or other row-level behavior by pruning the split
first.
Review Comment:
The safe-prefix check here is still driven by `_table_filters`, so it can
miss earlier unsafe conjuncts that do not reference any table slot.
`build_table_filters_from_conjunct()` only creates entries when
`collect_global_indices()` finds at least one `GlobalIndex`; a no-slot unsafe
expression such as `random()`/`uuid()` remains in the scanner's row-level
conjunct list but is absent from this loop. If such a conjunct appears before a
later partition/default-column predicate that maps to constants, this method
can prune the split and return before `Scanner::_filter_output_block()` ever
evaluates the unsafe conjunct on the non-empty split. Please preserve the
barrier from the original `_conjuncts` order, or carry slotless unsafe
conjuncts as non-executable barriers in `_table_filters`, and add a regression
test for an unsafe no-slot predicate before a false constant/partition
predicate.
##########
be/src/format_v2/timestamp_statistics.h:
##########
@@ -0,0 +1,62 @@
+// 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 <cctz/time_zone.h>
+
+#include <cstdint>
+
+#include "common/check.h"
+
+namespace doris::format {
+
+inline int64_t floor_epoch_seconds(int64_t value, int64_t units_per_second) {
+ DORIS_CHECK(units_per_second > 0);
+ auto seconds = value / units_per_second;
+ if (value < 0 && value % units_per_second != 0) {
+ --seconds;
+ }
+ return seconds;
+}
+
+// UTC instants are ordered, but converting them to civil DATETIMEV2 values is
not monotonic when
+// a timezone moves its clock backward. Metadata min/max converted across such
a transition cannot
+// safely form a ZoneMap: the true civil minimum or maximum may occur inside
the UTC interval.
+inline bool utc_timestamp_range_is_monotonic(int64_t min_seconds, int64_t
max_seconds,
+ const cctz::time_zone& timezone) {
+ DORIS_CHECK(min_seconds <= max_seconds);
Review Comment:
This assertion makes corrupt external metadata fatal before the callers can
fall back conservatively. The Parquet footer/page-index paths and the ORC
timestamp stats path pass metadata-derived endpoints into this helper; if a
file has inverted timestamp min/max values, the BE will hit this `DORIS_CHECK`
instead of treating those stats as unusable and scanning the row group/page or
falling back from aggregate pushdown. Please make `min_seconds > max_seconds`
return `false` here, or validate before calling, so invalid timestamp stats
follow the same conservative path as NaN, missing, or non-monotonic metadata.
--
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]
