Gabriel39 commented on code in PR #69646:
URL: https://github.com/apache/doris/pull/69646#discussion_r4385176933
##########
be/src/format_v2/table_reader.cpp:
##########
@@ -1614,6 +1615,15 @@ bool TableReader::_is_safe_to_pre_execute(const
VExprContextSPtr& conjunct) {
return predicate->is_safe_to_execute_on_selected_rows();
}
+bool TableReader::_is_safe_to_reorder_rows(const TableColumnMapper& mapper)
const {
+ // FE/scan conjuncts cannot see implicit materialization: an unrelated
DATE -> TIMESTAMP_NS
+ // output can overflow. Only direct mappings are proven safe, including
their nested children;
+ // a complex remap can run child casts even when its outer projection is a
SlotRef.
+ return std::ranges::all_of(_conjuncts, _is_safe_to_pre_execute) &&
+ std::ranges::all_of(mapper.mappings(),
Review Comment:
The premise here has been corrected: a valid TopN bound may skip evaluation
and materialization for rows that cannot contribute to the result. Preserving
potential exceptions on those excluded rows is not part of the intended
contract. No executed failure is suppressed.
The follow-up removes the overly restrictive projected-mapping proof rather
than extending it to more throwing-expression consumers. I added an actual
Iceberg v3/Parquet regression that projects only `k` and uses a nonmatching
TIMESTAMP_NS equality-delete key over retained DATE data. In both ASC and DESC,
a late bound excludes the invalid group and the expected surviving keys/counts
are returned. With no TopN filter, or with the reversed TopN direction that
requires the group, the hidden cast still reports TIMESTAMP_NS overflow. The PR
description now states this evaluation contract explicitly.
##########
be/src/format_v2/parquet/parquet_scan.cpp:
##########
@@ -1409,21 +1414,147 @@ std::vector<format::LocalColumnIndex>
ParquetScanScheduler::adaptive_predicate_p
return result;
}
+namespace {
+
+struct TopNOrderingColumn {
+ const VTopNPred* predicate;
+ const ParquetColumnSchema* schema;
+};
+
+std::optional<TopNOrderingColumn> find_topn_ordering_column(
+ const std::vector<std::unique_ptr<ParquetColumnSchema>>& file_schema,
+ const format::FileScanRequest& request) {
+ const size_t safe_count =
+ std::min(request.metadata_pruning_safe_conjunct_count,
request.conjuncts.size());
+ const VTopNPred* topn = nullptr;
+ for (size_t idx = 0; idx < safe_count; ++idx) {
+ const auto& root = request.conjuncts[idx]->root();
+ if (root->is_topn_filter() && root->can_evaluate_zonemap_filter()) {
+ topn = assert_cast<const VTopNPred*>(root.get());
+ break;
+ }
+ }
+ if (topn == nullptr) {
+ return std::nullopt;
+ }
+ const auto* slot = assert_cast<const VSlotRef*>(topn->get_child(0).get());
+ const auto position = std::ranges::find_if(request.local_positions,
[&](const auto& entry) {
+ return entry.second.value() == slot->column_id();
+ });
+ if (position == request.local_positions.end()) {
+ return std::nullopt;
+ }
+ const auto column = std::ranges::find_if(file_schema, [&](const auto&
schema) {
+ return schema->local_id == position->first.value();
+ });
+ if (column == file_schema.end() || (*column)->kind !=
ParquetColumnSchemaKind::PRIMITIVE ||
+ (*column)->max_repetition_level != 0 ||
+ !expr_zonemap::data_types_compatible((*column)->type,
slot->data_type())) {
+ return std::nullopt;
+ }
+ return TopNOrderingColumn {.predicate = topn, .schema = column->get()};
+}
+
+} // namespace
+
+void ParquetScanScheduler::prepare_topn_row_group_order(
+ const ParquetFileContext& file_context,
+ const std::vector<std::unique_ptr<ParquetColumnSchema>>& file_schema,
+ const format::FileScanRequest& request) {
+ _row_group_order_prepared = true;
+ const auto binding = find_topn_ordering_column(file_schema, request);
+ if (!binding.has_value()) {
+ return;
+ }
+ const auto* topn = binding->predicate;
+ const auto& schema = *binding->schema;
+ const size_t safe_count =
+ std::min(request.metadata_pruning_safe_conjunct_count,
request.conjuncts.size());
+ const auto& metadata = file_context.native_metadata->to_thrift();
+ _topn_footer_recheck = std::ranges::any_of(_scan_plan->row_groups,
[&](const auto& plan) {
+ const auto& group = metadata.row_groups[plan.row_group_id];
+ DORIS_CHECK(schema.leaf_column_id >= 0 && schema.leaf_column_id <
group.columns.size());
+ const auto& chunk = group.columns[schema.leaf_column_id];
+ return chunk.__isset.meta_data && chunk.meta_data.__isset.statistics;
+ });
+ // Files without statistics cannot benefit from repeated footer checks.
Row-ID fetches promise
+ // increasing positions, and an unsafe table expression must continue
observing file order.
+ if (!_topn_footer_recheck || request.row_ids.has_value() ||
!request.allow_row_reordering ||
Review Comment:
A materialization failure in a group that a valid TopN bound excludes does
not require reading that group. The intended guarantee is the final TopN
result; strict conversion rules still apply whenever values actually need to be
materialized. No conversion status is caught or discarded.
The added reader regression uses the exact DATE day value 2932897 in
physical RG0, ordinary dates in later groups, identity mappings and strict
mode. Both ASC and DESC skip RG0 only when its key cannot win and return the
expected keys/counts. An unfiltered scan and a reversed TopN that requires RG0
both retain the existing `Parquet DATE value is out of range` failure. This
behavior is now documented explicitly; a blanket native-conversion fence would
unnecessarily disable pruning.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/TopnFilterPushDownVisitor.java:
##########
@@ -108,9 +136,46 @@ public Boolean visitPhysicalProject(
return project.child().accept(this, ctxProjectProbeExpr);
}
+ private static boolean hasEvaluationSensitiveExpressions(Plan plan) {
+ return plan.getExpressions().stream().anyMatch(expression ->
!safeOnReducedInput(expression));
+ }
+
+ private static boolean safeOnReducedInput(Expression expression) {
+ // Determinism and monotonicity do not imply total evaluation:
from_second(-1), for example,
+ // throws. Keep a closed set of proven-total expressions instead of
enumerating throwing
+ // functions. Unknown functions, casts and UDFs must preserve their
original evaluation domain.
+ if (expression instanceof Slot || expression instanceof Literal) {
+ return true;
+ }
+ boolean safeNode = expression instanceof Alias || expression
instanceof Nullable
+ || expression instanceof And || expression instanceof Or ||
expression instanceof Not
+ || expression instanceof IsNull;
+ if (expression instanceof EqualTo || expression instanceof
NullSafeEqual
+ || expression instanceof LessThan || expression instanceof
LessThanEqual
+ || expression instanceof GreaterThan || expression instanceof
GreaterThanEqual
+ || expression instanceof InPredicate) {
+ safeNode = expression.children().stream().allMatch(child -> {
+ DataType type = child.getDataType();
+ return type.isNumericType() || type.isBooleanType() ||
type.isStringLikeType()
+ || type.isDateLikeType() || type.isTimeType() ||
type.isIPType()
+ || type.isUuidType() || type.isNullType();
+ });
+ } else if (expression instanceof Substring) {
+ safeNode = expression.children().subList(1,
expression.arity()).stream()
+ .allMatch(Literal.class::isInstance);
+ } else if (expression instanceof DateAddSubMonotonic) {
+ // Zero intervals are identities on already-typed dates; nonzero
arithmetic can overflow.
+ safeNode =
!ExpressionUtils.containsUnsafeDateArithmetic(expression);
+ }
+ return safeNode &&
expression.children().stream().allMatch(TopnFilterPushDownVisitor::safeOnReducedInput);
+ }
+
@Override
public Boolean visitPhysicalSetOperation(
PhysicalSetOperation setOperation, PushDownContext ctx) {
+ if (hasEvaluationSensitiveExpressions(setOperation)) {
Review Comment:
This example identifies a change in evaluation of excluded rows, not a
different final TopN result. Once the outer bound is k=0, k=100 cannot
contribute through the INTERSECT branch. The existing generated set filter may
consequently avoid evaluating B's conversion for that excluded key; preserving
a potential exception from it is not a requirement of this optimization.
I removed the error-totality whitelist and restored the original FE TopN
traversal logic and ExpressionUtils (with an explanatory traversal comment).
There is no new set-operation rewrite or runtime-filter generation policy in
the final production diff. The PR now documents the evaluation contract, and
the FE regression verifies that potentially throwing secondary expressions do
not unnecessarily disable existing TopN filters. The remaining production
change is confined to TopN ZoneMap evaluation and Parquet scheduling. No full
SQL reproduction of the proposed scheduling interleaving is claimed.
--
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]
