github-actions[bot] commented on code in PR #66681:
URL: https://github.com/apache/doris/pull/66681#discussion_r3774650337
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/ReorderJoin.java:
##########
@@ -153,7 +175,10 @@ public Plan joinToMultiJoin(Plan plan, Map<Plan,
DistributeHint> planToHintType)
// (t1 join t2) join t3 where t1.a = t3.x + random()
// if reorder, then may have ((t1 join t3) on t1.a = t3.x +
random()) join t2,
// then the reorder result will less rows than origin.
- if (conjunct.containsVolatileExpression()) {
+ // a NoneMovableFunction (e.g. assert_true) has the same
problem: reordering
+ // turns the filter into a join condition that is evaluated on
a different
+ // (superset) row set, which changes its error behavior or
results.
+ if (conjunct.containsNoneMovableOrVolatile()) {
return plan;
Review Comment:
**[P1] Preserve NoneMovable predicates already stored on join edges**
This check covers only conjuncts on the surrounding `LogicalFilter`.
Ordinary inner-join ON predicates still enter both reorder implementations. In
hypergraph-v2, `HyperGraph.addJoin` splits every conjunct onto the minimal
referenced node pair; for example:
```text
Join(other: assert_true(A.x = C.x))
Join(hash: A.k = B.k)
A
B
C
```
can be enumerated as `Join(hash A-B, Join(other assert A-C, A, C), B)`,
evaluating the assertion before a nonmatching B join that originally eliminated
all rows. The classical MultiJoin path has the same issue in a three-way
cluster because it can prefer a later hash-connected child over an
assertion-only edge. Please make a join containing a volatile/NoneMovable hash
or other conjunct a boundary (or keep that conjunct on its original full edge)
in both reorder paths.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/ProjectOtherJoinConditionForNestedLoopJoinTest.java:
##########
@@ -79,4 +82,27 @@ public void testHashJoin() {
)
).printlnTree();
}
+
+ /**
+ * A join condition containing a NoneMovableFunction (assert_true) must
stay inline in the
+ * other join conjuncts: aliasing it into a child project would change its
evaluation
+ * granularity from per join pair to per row of that child.
+ */
+ @Test
+ public void testNoneMovableConditionStaysInline() {
+ Slot a = scan1.getOutput().get(1);
+ Slot b = scan2.getOutput().get(0);
+ Expression otherCondition = new AssertTrue(
+ new LessThan(a, new Add(b, b)), new StringLiteral("msg"));
Review Comment:
**[P2] Exercise the side-local branch changed by this patch**
This `AssertTrue(a < b + b)` references both join children. Before the new
guard, neither child's slot set covered the outer expression, so the rewriter
already recursed and left the outer `AssertTrue` inline; only the deterministic
`b + b` subexpression could be projected. Therefore the current matcher passes
against the pre-change implementation.
Please use a wholly side-local condition such as `assert_true(a > 0,
'msg')`, assert that neither child project contains the assertion, and include
a deterministic side-local control that is still projected.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/ExpressionTrait.java:
##########
@@ -139,4 +139,16 @@ default boolean isVolatile() {
default boolean containsVolatileExpression() {
return containsType(VolatileExpression.class) && anyMatch(expr ->
((ExpressionTrait) expr).isVolatile());
}
+
+ /**
+ * Identify whether the expression contains a volatile expression or a
NoneMovableFunction.
+ * Both kinds must not be moved, duplicated or pruned by rewrite rules: a
volatile expression
+ * (e.g. rand(), uuid()) is non-deterministic, and a NoneMovableFunction
(e.g. assert_true)
+ * has side effects (throws errors). Relocating either to a different row
domain, or
+ * duplicating its evaluation, changes query semantics or error behavior,
so rules should
+ * treat both identically (keep them above row-changing operators, never
clone them).
+ */
Review Comment:
**[P1] Apply this contract to active Sort and constant-UNION rewrites**
Two registered paths still violate the contract documented here:
- `PushDownFilterThroughSort` unconditionally changes `Limit ->
Filter(assert_true) -> Sort -> Scan` to `Limit -> Sort -> Filter -> Scan`. The
blocking Sort now evaluates a bad input row before producing the safe prefix
that the original Limit could consume.
- After `MergeOneRowRelationIntoUnion`, `PushProjectIntoUnion` uses
volatile-only admission. `Project(1 AS y) -> UnionAll(const assert_true(false)
AS x, const true)` can therefore drop the unreferenced assertion and turn a
required error into returned rows.
Please complete the registration-wide fence using
`containsNoneMovableOrVolatile()` and add Sort-plus-Limit and constant-UNION
pruning regressions.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownFilterThroughAggregation.java:
##########
@@ -69,7 +69,11 @@ public Rule build() {
// 2. if the conjunct contains unique function, it should not
be pushed down;
// e.g. 'select a, sum(a) from t group by a having a +
random() > 10'
// not equals 'select a, sum(a) from t where a + random() >
10 group by a'
- if (!conjunct.containsVolatileExpression()
+ // 3. a NoneMovableFunction (e.g. assert_true) must not be
pushed below the
+ // aggregation either: the aggregation changes which rows
are evaluated
+ // (grouped output vs input rows), so assert_true would run
on a different
+ // domain and its error behavior would change.
+ if (!conjunct.containsNoneMovableOrVolatile()
&& !conjunctSlots.isEmpty() &&
canPushDownSlots.containsAll(conjunctSlots)) {
Review Comment:
**[P1] Preserve the entire filter boundary around NoneMovable conjuncts**
Blocking only the assertion conjunct is insufficient when a sibling is
moved. For example:
```text
Filter(assert_true(k > 0, 'bad'), k = 1)
Aggregate(group by k)
Scan(k = -1, 1)
```
becomes `Filter(assert_true) -> Aggregate -> Filter(k=1) -> Scan`, so the
failing `k=-1` group disappears before the assertion. The same per-conjunct
partitioning exists in several changed join/CTE/set-op/window/generate paths,
and some new mixed tests codify that partial movement.
The inverse is also unsafe: registered `MergeFilters` collapses
`Filter(assert_true) -> Filter(k=1)`; BE evaluates each conjunct expression on
the same full block before combining masks, newly exposing the assertion to
`k=-1`. Please treat any filter containing a NoneMovable conjunct as a whole
boundary across row-domain-changing rewrites and filter merging, with
mixed-conjunct runtime regressions.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/eageraggregation/EagerAggRewriter.java:
##########
@@ -784,7 +784,7 @@ public Plan visitLogicalFilter(LogicalFilter<? extends
Plan> filter, PushDownAgg
if (context.aggFuncAndGroupKeyAllEmpty() ||
context.hasVolatileFunctions()) {
return filter;
}
- if
(filter.getConjuncts().stream().anyMatch(Expression::containsVolatileExpression))
{
+ if
(filter.getConjuncts().stream().anyMatch(Expression::containsNoneMovableOrVolatile))
{
return genAggregate(filter, context);
Review Comment:
**[P1] Fence NoneMovable aggregate arguments, not only filters**
The new check protects a `LogicalFilter`, but eager-aggregation admission
and context checks still reject only `containsVolatileExpression()`. A
reachable tree is:
```text
Aggregate(group=t2.id, count(assert_true(t1.id > 0, 'bad')))
InnerJoin(t1.id = t2.id)
Scan t1
Scan t2
```
With eager aggregation enabled, `count(assert_true(...))` can be generated
below the join on the `t1` child. A failing `t1` row with no matching `t2` row
is never evaluated in the original tree, but it throws in the pushed child
aggregate. Please apply the combined fence at aggregate-function admission and
every context/project recheck, and add an unmatched-row regression; the new
filter-only test cannot cover this path.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/ProjectOtherJoinConditionForNestedLoopJoin.java:
##########
@@ -114,7 +114,9 @@ public Expression visit(Expression expression,
ReplacerContext ctx) {
// pair" to "per row of that child", which silently changes
results. Keep such
// expressions inline in otherJoinConjuncts, but still recurse to
extract deterministic
// child expressions.
- if (expression.containsVolatileExpression()) {
+ // A NoneMovableFunction (e.g. assert_true) has the same
granularity problem, so it
+ // must stay inline as well.
+ if (expression.containsNoneMovableOrVolatile()) {
return super.visit(expression, ctx);
Review Comment:
**[P1] Guard equal ON predicates before hash classification**
This guard runs after `FindHashConditionForJoin`. The shared
`JoinUtils.isHashJoinCondition` still rejects only volatile expressions, so an
ON predicate such as
```text
NestedLoopJoin(other: t1.flag = assert_true(t2.v > 0, 'bad'))
Empty t1
Scan t2
```
is first promoted to a hash conjunct; `PushDownExpressionsInHashCondition`
then materializes `assert_true` in the right-child project. The original nested
loop has zero candidate pairs and does not evaluate the assertion, while the
hash build evaluates the failing `t2` row and throws. Please use
`containsNoneMovableOrVolatile()` in the shared hash classifier (and
defensively in hash-operand materialization), with an empty-probe regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownProjectThroughLimit.java:
##########
@@ -49,10 +49,15 @@ public class PushDownProjectThroughLimit extends
OneRewriteRuleFactory {
@Override
public Rule build() {
- return logicalProject(logicalLimit()).thenApply(ctx -> {
- LogicalProject<LogicalLimit<Plan>> logicalProject = ctx.root;
- LogicalLimit<Plan> logicalLimit = logicalProject.child();
- return
logicalLimit.withChildren(logicalProject.withChildren(logicalLimit.child()));
- }).toRule(RuleType.PUSH_DOWN_PROJECT_THROUGH_LIMIT);
+ // a project that computes a NoneMovableFunction (e.g. assert_true)
must not be
+ // pushed below the limit: the limit prunes rows, so assert_true would
be evaluated
+ // on a different (larger) domain and its error behavior would change.
+ return logicalProject(logicalLimit())
+ .whenNot(LogicalProject::containsNoneMovableFunction)
+ .thenApply(ctx -> {
Review Comment:
**[P1] Fence both project-relocation directions and both expression kinds**
The new predicate covers only `NoneMovableFunction` in this direction.
`Project(random(1) AS r) -> Limit(1 OFFSET 10) -> Scan` is still pushed below
Limit, advancing the seeded sequence on discarded rows and changing the
retained value.
Conversely, registered `PullUpProjectUnderLimit`, `PullUpProjectUnderTopN`,
and `PullUpProjectBetweenTopNAndAgg` do not check NoneMovable expressions. A
matching `Limit -> Project(assert_true(...)) -> CrossJoin` becomes
`Project(assert_true(...)) -> Limit -> CrossJoin`, so discarded join rows stop
evaluating the assertion. Please use the combined volatile/NoneMovable fence
here and on all inverse whole-project pull-ups; the existing inverse Limit/TopN
tests are disabled.
--
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]
