github-actions[bot] commented on code in PR #66681:
URL: https://github.com/apache/doris/pull/66681#discussion_r3841413019
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushProjectIntoUnion.java:
##########
@@ -101,29 +98,18 @@ private boolean
canPushProjectIntoUnion(LogicalProject<LogicalUnion> project) {
return false;
}
for (List<NamedExpression> constExprs : union.getConstantExprsList()) {
- Set<Slot> uniqueFunctionSlots = Sets.newHashSet();
- for (int i = 0; i < constExprs.size(); i++) {
- NamedExpression ne = constExprs.get(i);
- if (ne.containsVolatileExpression()) {
- uniqueFunctionSlots.add(union.getOutput().get(i));
- }
- }
- if (uniqueFunctionSlots.isEmpty()) {
- continue;
- }
- Set<Slot> counterSet = Sets.newHashSet();
- // for a union slot which contains unique function, if it exists
in project multiple times,
- // then don't push project into union, otherwise the unique
function will be copy multiple times.
- // e.g. `select a as b, a as c from (select random() as a union
all select 2 as a)`
- // if push down the project, then random() will be evaluated
twice: `random() as b, random() as c`
- for (NamedExpression ne : project.getProjects()) {
- if (ne.anyMatch(expr -> expr instanceof Slot
- && uniqueFunctionSlots.contains(expr) &&
!counterSet.add((Slot) expr))) {
+ for (NamedExpression ne : constExprs) {
+ // reject sensitive constant rows wholesale: a
NoneMovableFunction (e.g.
+ // assert_true) or a volatile constant must never be pushed
into the union.
+ // substitution plus constant folding can eliminate the
expression entirely
+ // (e.g. IF(FALSE, assert_true(...), TRUE) -> TRUE), suppress
a required error,
+ // or duplicate/copy its evaluation, even when the parent
project references it
+ // only once.
+ if (ne.containsNoneMovableOrVolatile()) {
Review Comment:
**[P1] Fence sensitive expressions owned by the parent Project**
This admission check now rejects sensitive UNION cells, but it never
inspects the expressions owned by `project`. Ordinary SQL can reach
`Project(random(1) AS r) -> UnionAll(const 1; const 2)`: the earlier
`PushProjectThroughUnion` cannot push this computed projection, then this rule
creates a new Alias containing `random(1)` for every constant row.
`PhysicalPlanTranslator` translates those row expressions independently, so the
original single seeded context producing `r1, r2` becomes two contexts both
producing `r1`. Please reject the rule when any parent project expression
contains `containsNoneMovableOrVolatile()` (or retain one shared Project), and
add a full-Rewriter/runtime test over multiple constant rows. This is distinct
from the existing comments about sensitive expressions already stored in UNION
cells or regular children.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/InferPredicates.java:
##########
@@ -144,9 +145,16 @@ public Plan visitLogicalFilter(LogicalFilter<? extends
Plan> filter, JobContext
filter = visitChildren(this, filter, context);
Set<Expression> inferredPredicates = pullUpPredicates(filter);
inferredPredicates.removeAll(pullUpAllPredicates(filter.child()));
- if (inferredPredicates.isEmpty()) {
+ // NoneMovableFunction (e.g. assert_true) and volatile conjuncts are
not pulled up by
+ // PullUpPredicates; keep them so the filter (and its
error/side-effect behavior) is
+ // preserved at its original position.
+ Set<Expression> noneMovableConjuncts = filter.getConjuncts().stream()
+ .filter(Expression::containsNoneMovableOrVolatile)
Review Comment:
**[P1] Preserve sensitive siblings before constant-FALSE elimination**
The new restoration runs after `visitLogicalFilter`'s `contains(FALSE)`
early return. Thus `Filter(assert_true(k > 0, 'bad'), FALSE) -> Scan(k=-1)` is
replaced by `LogicalEmptyRelation` before `noneMovableConjuncts` is collected,
suppressing the required error. The earlier registered `EliminateFilter` has
the same FALSE/NULL deletion path, so fixing only this block would still leave
the query wrong. Please inspect and preserve sensitive siblings before both
eliminations and add a full registered-stage/runtime regression for FALSE and
NULL. Existing filter threads cover movement/merging, not whole-filter deletion
into an empty relation.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/TransposeSemiJoinLogicalJoin.java:
##########
@@ -44,6 +45,15 @@ public Rule build() {
|| topJoin.left().getJoinType().isRightOuterJoin())))
.whenNot(topJoin -> topJoin.hasDistributeHint() ||
topJoin.left().hasDistributeHint())
.whenNot(topJoin -> topJoin.isLeadingJoin() ||
topJoin.left().isLeadingJoin())
+ // the transpose moves the top semi join's conjuncts (with the
A-C match) below the
+ // bottom join, and the bottom join's conjuncts above the semi
join: a
+ // NoneMovableFunction (e.g. assert_true) or volatile
expression owned by either
+ // join would be evaluated on a different (superset or pruned)
row set, changing its
+ // error behavior or results. reject the transpose.
+ .whenNot(topJoin -> topJoin.getExpressions().stream()
Review Comment:
**[P1] Fence the probe subtree gated by the inverse transpose**
These new checks cover only expressions owned by the top and bottom joins.
On the RIGHT branch, `SemiJoin(InnerJoin(Filter(assert_true(...))->A, B), C)`
becomes `InnerJoin(A, SemiJoin(B,C))`. With nonempty B and C but no B-C match,
the original build sides pull A and raise; the new empty right build
short-circuits the enclosing INNER join without pulling A. The slot-Project
sibling is registered alongside this rule and has the same gap. Please
recursively fence the child whose execution becomes gated in each branch and
test the RIGHT empty-build case in both factories. Existing inverse-transpose
comments cover join-owned expressions, not this ordinary child carrier.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/joinorder/hypergraphv2/HyperGraph.java:
##########
@@ -103,8 +103,29 @@ public static boolean isValidJoin(Plan plan) {
return false;
}
LogicalJoin<?, ?> join = (LogicalJoin<?, ?>) plan;
- return !join.isMarkJoin() && !join.isLeadingJoin() &&
!join.hasDistributeHint()
- && !join.getJoinType().isAsofJoin();
+ if (join.isMarkJoin() || join.isLeadingJoin() ||
join.hasDistributeHint()
+ || join.getJoinType().isAsofJoin()) {
+ return false;
+ }
+ // a join whose own hash/other conjunct contains a NoneMovableFunction
(e.g. assert_true)
+ // or a volatile expression is a boundary: HyperGraph.addJoin would
split the conjunct
+ // onto the minimal referenced node pair and let DPHyp evaluate it
before other joins
+ // that originally eliminated the rows, changing its error behavior or
results.
+ return !hasNoneMovableOrVolatileConjunct(join);
Review Comment:
**[P1] Treat sensitive input groups as join-order boundaries**
`isValidJoin` now checks only expressions owned by each join. For
`(Filter(assert_true(A.v > 0, 'bad'))->A join B) join C`, choose nonempty B and
C with no B-C match. The original two nonempty build sides pull A and raise;
DPHyper can instead enumerate `A join (B join C)`, where the empty B-C build
makes the outer INNER hash join return EOS without ever pulling A
(`short_circuit_for_probe`). The classical association/exchange guards have the
same owner-only omission. Please reject reordering across recursively sensitive
input groups with a memo-safe traversal, and add the three-input empty-build
runtime case. Existing threads cover sensitive join-edge conjuncts, not a
deterministic join around a sensitive input node.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/ColumnPruning.java:
##########
@@ -410,7 +410,8 @@ private LogicalUnion pruneUnionOutput(LogicalUnion union,
PruneContext context)
List<Integer> extractColumnIndex = Lists.newArrayList();
for (int i = 0; i < originOutput.size(); i++) {
NamedExpression output = originOutput.get(i);
- if (context.requiredSlotsIds.contains(output.getExprId().asInt()))
{
+ if (context.requiredSlotsIds.contains(output.getExprId().asInt())
+ || isSensitiveConstantColumn(i, constantExprsList)) {
Review Comment:
**[P1] Preserve sensitive outputs outside constant UNION cells**
This fixes only sensitive constant cells, while the same registered pass
still prunes other owners: `pruneAggregate` drops an unused
`count(assert_true(...))`; `visitLogicalWindow` can remove an unused sensitive
window and then the whole operator; generic `OutputPrunable` can remove a
sensitive `LogicalOneRowRelation` column; and when no UNION column is required,
the branch below replaces regular child Projects with `Project(1)`, bypassing
`LogicalProject.pruneOutputs` restoration. Derived-table queries therefore
change from raising to returning rows or counts. Please make sensitive-output
preservation a shared invariant across pruning implementations and regular
UNION sources, and add full-Rewriter/runtime cases. This is distinct from the
existing comment about a sensitive constant UNION cell, which this hunk
addresses.
--
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]
