github-actions[bot] commented on code in PR #69776:
URL: https://github.com/apache/doris/pull/69776#discussion_r4478513325
##########
be/src/runtime/query_context.cpp:
##########
@@ -66,6 +66,33 @@ class DelayReleaseToken : public Runnable {
std::unique_ptr<ThreadPoolToken> token_;
};
+void QueryContext::init_runtime_predicates(
+ const std::vector<TTopnFilterDesc>& topn_filter_descs,
+ const std::vector<TPipelineFragmentParams>& fragment_params) {
+ std::unordered_set<TPlanNodeId> strict_sources;
+ for (const auto& params : fragment_params) {
+ for (const auto& node : params.fragment.plan.nodes) {
+ if (node.node_type == TPlanNodeType::SORT_NODE) {
+ const auto& sort = node.sort_node;
+ // A heap retains LIMIT + OFFSET candidates before publishing
a bound. With one
+ // key, another boundary tie cannot improve them. Multiple
keys must keep ties:
+ // (1, 2) can still beat (1, 9). Missing algorithms execute as
FULL_SORT.
+ if (sort.__isset.algorithm && sort.algorithm ==
TSortAlgorithm::HEAP_SORT &&
+ sort.sort_info.ordering_exprs.size() == 1) {
Review Comment:
[P1] Require sender opt-in before enabling strict TopN bounds
An older FE already sends `HEAP_SORT` and one order key, so this branch
enables strict mode for its descriptors even though it lacks this PR's
aggregate/join/volatile safety guards. This is reachable during the documented
[BE-before-FE upgrade
sequence](https://doris.apache.org/docs/4.x/admin-manual/cluster-management/upgrade/#upgrade-flow-overview).
For example, the pre-change FE can produce this reduced plan with
TopN-through-UNION/JOIN rewrites disabled:
```text
TopN(k ASC NULLS LAST, LIMIT 1)
UnionAll
Scan(sibling: k=1, payload=999)
RIGHT ANTI JOIN(a.k=b.k)
Scan(a: k=1)
Scan(b: k=1, payload=7)
```
That FE substitutes `b.k` with `a.k` and registers the left scan target. If
the sibling publishes bound 1 first, the new BE applies `a.k < 1`, removes the
match and invents `(1,7)` from the originally empty anti branch. `HeapSorter`
accepts that equal-key block and can evict the retained sibling while trimming,
returning the invalid row. The previous inclusive `<= 1` preserves the match;
the new FE's guard prevents the descriptor, but an old FE still sends it.
Default older descriptors to inclusive comparisons and require an explicit
safe-sender capability or equivalent version gate; add an old-FE
descriptor/new-BE regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/TopnFilterPushDownVisitor.java:
##########
@@ -191,6 +201,25 @@ private PushDownContext
adjustProbeExprNullableThroughOuterJoin(Plan leftChild,
return ctx;
}
+ private boolean canPushDownToLeftChild(AbstractPhysicalJoin<? extends
Plan, ? extends Plan> join,
+ Expression probeExpr) {
+ JoinType joinType = join.getJoinType();
+ if (joinType.isRightAntiJoin() || (joinType.isRightSemiJoin() &&
join.isMarkJoin())) {
+ // Removing a probe can create an unmatched right row or change
its mark value.
+ // This also applies to constant probes, which have no input slots.
+ return false;
+ }
+ if (joinType.isRightOuterJoin() || joinType.isFullOuterJoin() ||
joinType.isAsofRightOuterJoin()) {
+ // A right-side ordering key keeps its value when a match is
removed, but the left
+ // payload changes. Strict bounds can then return a row that the
original join would
+ // not produce. Slot-free expressions have the same problem. Check
the original
+ // probe before substituting a right equality key with its left
counterpart.
+ Set<Slot> inputSlots = probeExpr.getInputSlots();
+ return !inputSlots.isEmpty() &&
join.left().getOutputSet().containsAll(inputSlots);
Review Comment:
[P1] Reject non-null-preserving ranks across right/full outer joins
Left-only input slots do not make a probe safe when NULL extension can
improve its value. Consider this reduced same-fragment plan:
```text
TopN(rk ASC NULLS LAST, LIMIT 1)
Project(COALESCE(a.k, 0) AS rk, a.v, b.id)
RIGHT OUTER JOIN(a.id = b.id)
Scan(a)
Scan(b)
```
If every row matches and every `a.k` is 1, an initial probe batch fills the
heap and publishes bound 1. This guard admits `COALESCE(a.k,0)`, so the new
strict scan predicate `< 1` removes later matching rows.
`ProcessHashTableProbe::finish_probing` then emits their right rows with NULL
left columns, producing rank 0, which wins TopN despite being absent from the
original result. The previous `<= 1` retained those ties. FULL OUTER and the
nested-loop caller have the same problem. Require the complete probe to remain
NULL on NULL-extended left inputs, or stop pushdown here; add a regression for
this left-expression case.
--
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]
