github-actions[bot] commented on code in PR #66681:
URL: https://github.com/apache/doris/pull/66681#discussion_r3803661013
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushProjectIntoUnion.java:
##########
@@ -102,23 +103,36 @@ private boolean
canPushProjectIntoUnion(LogicalProject<LogicalUnion> project) {
}
for (List<NamedExpression> constExprs : union.getConstantExprsList()) {
Set<Slot> uniqueFunctionSlots = Sets.newHashSet();
+ Set<Slot> noneMovableSlots = 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 (ne.containsType(NoneMovableFunction.class)) {
+ noneMovableSlots.add(union.getOutput().get(i));
+ }
}
- if (uniqueFunctionSlots.isEmpty()) {
+ Set<Slot> guardedSlots = Sets.union(uniqueFunctionSlots,
noneMovableSlots);
+ if (guardedSlots.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.
+ // for a union slot which contains unique function or a
NoneMovableFunction, if it exists
+ // in project multiple times, then don't push project into union,
otherwise the expression
+ // will be copied 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))) {
+ && guardedSlots.contains(expr) &&
!counterSet.add((Slot) expr))) {
Review Comment:
**[P1] Preserve a single sensitive reference through folding**
This counter treats one syntactic occurrence as safe, but this rule
substitutes each constant row and immediately folds the result. A registered
reachable shape is:
```text
Project(IF(c, x, TRUE))
UnionAll(row c=FALSE, x=assert_true(FALSE, 'bad'); row c=TRUE, x=TRUE)
```
The one-output Project fails the earlier `PushProjectThroughUnion`
output-count prerequisite, then reaches this check. `x` is recorded exactly
once, but substitution produces `IF(FALSE, assert_true(FALSE, 'bad'), TRUE)`
and `FoldConstantRuleOnFE.visitIf` reduces it to `TRUE`. The original constant
UNION materializes `x` below the Project and errors, so the rewrite suppresses
a required error. This is distinct from the existing zero-reference and
repeated-reference threads. Please reject sensitive constant rows wholesale, or
admit only a bare slot/direct alias forwarding shape, and add a full-stage
conditional regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/join/LogicalJoinSemiJoinTransposeProject.java:
##########
@@ -50,7 +50,14 @@ public List<Rule> buildRules() {
|| topJoin.getJoinType().isLeftOuterJoin())))
.whenNot(topJoin -> topJoin.hasDistributeHint()
|| topJoin.left().child().hasDistributeHint())
- .when(join -> join.left().isAllSlots()))
+ .when(join -> join.left().isAllSlots())
+ // the transpose moves the bottom semi join's
conjuncts and its RIGHT
+ // subtree above the new bottom join (built from the
top join, which
+ // prunes rows when it is inner): a
NoneMovableFunction (assert_true)
+ // or volatile expression there would be evaluated on
fewer rows and
+ // its required error could be suppressed, so the
transpose must be
+ // rejected in that case
+ .whenNot(topJoin ->
isBottomSemiSensitive(topJoin.left().child())))
Review Comment:
**[P1] Fence sensitive expressions owned by the top join**
This predicate checks only the old bottom semi/anti join, but the
transformation installs the old top join as the new lower join. For example:
```text
InnerJoin(other: assert_true(A.v + C.v > 0))
Project
LeftSemiJoin(A, B)
C
```
If B removes the failing A row, the original top assertion sees only safe
rows. After `topJoin.withChildrenNoContext(a, c)`, that assertion runs on A-C
before the rebuilt top semi join can prune with B, turning a successful query
into an error. The right-hand and `SemiJoinSemiJoinTransposeProject`
constructions have the same reversal, even when the intervening Project
forwards slots only. Please fence every expression list of the old top join in
both transpose families and add left/right plus semi-over-semi base-failing
tests.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/TransposeAggSemiJoinProject.java:
##########
@@ -37,6 +37,12 @@ public class TransposeAggSemiJoinProject extends
OneExplorationRuleFactory {
public Rule build() {
return logicalAggregate(logicalProject(
logicalJoin().when(join ->
join.getJoinType().isLeftSemiOrAntiJoin() && !join.isMarkJoin())))
+ // the transpose moves the project from above the semi/anti
join (where it is
+ // evaluated on the pruned rows) to below the aggregate (where
it is evaluated
+ // on all left rows, before the join prunes): a
NoneMovableFunction (assert_true)
+ // or volatile expression there would run on rows the join
removes, turning
+ // returned rows into errors. reject the transpose then.
+ .whenNot(agg -> agg.child().containsNoneMovableOrVolatile())
Review Comment:
**[P1] Fence Aggregate- and join-owned expressions too**
The Project can be slot-only while another moved expression is sensitive. A
deterministic normalized case is:
```text
Aggregate(group k, count(assert_true(v > 0, 'bad')))
Project(k, v)
LeftSemiJoin(A, B)
```
With a failing unmatched A row and B containing only the safe key, the
original semi join removes the failing row before the aggregate argument runs.
The generated alternative aggregates A first and errors. `NormalizeAggregate`
can leave the assertion inside `count(...)` while pushing only its input slots
into this Project, so the new check passes. Separately, a volatile semi/anti
join conjunct changes from per-input-row evaluation to per-aggregate-group
evaluation. The projected exploration rule and both registered inverse
`TransposeSemiJoinAgg*` rewrites share these gaps. Please reject every
direction when either the Aggregate expressions or any join expression list is
sensitive, and test both carriers.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/PushProjectIntoUnionTest.java:
##########
@@ -118,6 +122,82 @@ public void
testConstantExprIdsDistinctFromUnionOutputAndAcrossRows() {
}
}
+ /**
+ * A constant UNION row holding a NoneMovableFunction (assert_true(false))
must never be
+ * dropped by pushing a parent project that does not reference it: the
push-down would turn a
+ * required assertion/error into plain returned rows. the rule must not
fire.
+ */
+ @Test
+ public void testDoNotPushProjectIntoUnionWithNoneMovableConst() {
+ SlotReference s = new SlotReference(new ExprId(10), "s",
+ IntegerType.INSTANCE, true, ImmutableList.of());
+ SlotReference x = new SlotReference(new ExprId(11), "x",
+ BooleanType.INSTANCE, true, ImmutableList.of());
+ // constant row: s = 1, x = assert_true(false) — a required assertion
that throws.
+ NamedExpression rowS = new Alias(new ExprId(1), new IntegerLiteral(1),
"1");
+ NamedExpression rowX = new Alias(new ExprId(2), new AssertTrue(
+ BooleanLiteral.of(false), new StringLiteral("msg")), "x");
+ LogicalUnion union = new LogicalUnion(Qualifier.ALL,
+ ImmutableList.of(s, x),
+ ImmutableList.of(),
+ ImmutableList.of(ImmutableList.of(rowS, rowX)),
+ false,
+ ImmutableList.of());
+ // parent project selects only s, dropping x: pushing it into the
union would drop the assertion.
+ Alias parentAlias = new Alias(new ExprId(100), s, "y");
+ LogicalProject<LogicalUnion> project = new LogicalProject<>(
+ ImmutableList.<NamedExpression>of(parentAlias), union);
+
+ Plan rewritten =
PlanChecker.from(MemoTestUtils.createConnectContext(), project)
+ .applyTopDown(new PushProjectIntoUnion())
+ .getPlan();
+
+ // the rule must not fire: the project stays above the union.
+ Assertions.assertTrue(rewritten instanceof LogicalProject,
rewritten.treeString());
+ Assertions.assertTrue(((LogicalProject<?>) rewritten).child()
instanceof LogicalUnion,
+ rewritten.treeString());
+ }
+
+ /**
+ * A project that references the assertion-backed UNION slot twice must
never be pushed into
+ * the union: the push-down would copy the NoneMovableFunction and
evaluate it twice,
+ * violating the no-duplication contract. this case fails on a
volatile-only guard (the
+ * assertion is not volatile, so the guarded-slots check never triggers);
the NoneMovable
+ * guard must reject it. the rule must not fire.
+ */
+ @Test
+ public void
testDoNotPushProjectIntoUnionWithNoneMovableConstReferencedTwice() {
+ SlotReference s = new SlotReference(new ExprId(10), "s",
+ IntegerType.INSTANCE, true, ImmutableList.of());
+ SlotReference x = new SlotReference(new ExprId(11), "x",
+ BooleanType.INSTANCE, true, ImmutableList.of());
+ // constant row: s = 1, x = assert_true(false) — a required assertion
that throws.
+ NamedExpression rowS = new Alias(new ExprId(1), new IntegerLiteral(1),
"1");
+ NamedExpression rowX = new Alias(new ExprId(2), new AssertTrue(
+ BooleanLiteral.of(false), new StringLiteral("msg")), "x");
+ LogicalUnion union = new LogicalUnion(Qualifier.ALL,
+ ImmutableList.of(s, x),
+ ImmutableList.of(),
+ ImmutableList.of(ImmutableList.of(rowS, rowX)),
+ false,
+ ImmutableList.of());
+ // parent project references x twice: pushing it into the union would
copy the assertion.
+ LogicalProject<LogicalUnion> project = new LogicalProject<>(
+ ImmutableList.<NamedExpression>of(
+ new Alias(new ExprId(100), x, "a"),
+ new Alias(new ExprId(101), x, "b")),
+ union);
+
+ Plan rewritten =
PlanChecker.from(MemoTestUtils.createConnectContext(), project)
+ .applyTopDown(new PushProjectIntoUnion())
Review Comment:
**[P1] Exercise the registered UNION pruning path**
This direct `applyTopDown(new PushProjectIntoUnion())` check proves only the
local guard. In the registered pipeline, after this rule preserves
```text
Project(s)
UnionAll(row s=1, x=assert_true(FALSE, 'bad'))
```
later `ColumnPruning.pruneUnionOutput` keeps only the required `s` index and
deletes the assertion-bearing constant cell. The original UNION materializes
`x` and errors; the final physical UNION never sees it. The earlier registered
`PushProjectThroughUnion` is also a separate cloning bypass for repeated
sensitive inputs. Please preserve sensitive constant indexes during UNION
pruning, apply the same fence to the earlier cloning rule, and exercise the
full registered `Rewriter` stage rather than this rule alone.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/JoinUtils.java:
##########
@@ -478,4 +479,48 @@ public static boolean
checkReorderPrecondition(LogicalJoin<?, ?> top, LogicalJoi
return AdjustNullable.doVisitLogicalJoin(
join, equalConjunctsSlotMap, false, false);
}
+
+ /**
+ * whether any hash or other conjunct of the join contains a
NoneMovableFunction (e.g.
+ * assert_true) or a volatile expression. such conjuncts must not be moved
onto a different
+ * join edge by join reorder rules: they would be evaluated on a different
(superset or
+ * pruned) row set, which changes their error behavior or results.
+ */
+ public static boolean hasSensitiveConjunct(LogicalJoin<?, ?> join) {
+ for (Expression conjunct : join.getHashJoinConjuncts()) {
+ if (conjunct.containsNoneMovableOrVolatile()) {
+ return true;
+ }
+ }
+ for (Expression conjunct : join.getOtherJoinConjuncts()) {
+ if (conjunct.containsNoneMovableOrVolatile()) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * whether any logical expression of the group contains a
NoneMovableFunction (e.g.
+ * assert_true) or a volatile expression.
+ */
+ public static boolean groupContainsSensitiveExpression(GroupPlan
groupPlan) {
+ return groupPlan.getGroup().getLogicalExpressions().stream()
+ .anyMatch(groupExpression ->
planContainsSensitiveExpression(groupExpression.getPlan()));
+ }
+
+ private static boolean planContainsSensitiveExpression(Plan plan) {
+ if
(plan.getExpressions().stream().anyMatch(Expression::containsNoneMovableOrVolatile))
{
+ return true;
+ }
+ for (Plan child : plan.children()) {
Review Comment:
**[P1] Descend through memo child groups**
`Memo.init` replaces ordinary children with `GroupPlan`s. Thus a normal
column-pruned RHS such as
```text
Project(B.x)
Filter(assert_true(B.y > 0, 'bad'))
ScanB
```
becomes a harmless Project group expression whose child is
`GroupPlan(filterGroup)`. This helper sees only `B.x`, deliberately refuses to
expand that child, and returns false without visiting the sensitive Filter.
Both new semi-transpose RHS fences then admit the unsafe alternative; their
direct tests cover only a sensitive group root. Please walk child groups
recursively with a visited group-id set and add a wrapper-hidden RHS case for
both callers.
--
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]
