Gabriel39 commented on code in PR #69646:
URL: https://github.com/apache/doris/pull/69646#discussion_r4384486597
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/TopnFilterPushDownVisitor.java:
##########
@@ -192,6 +202,10 @@ private PushDownContext
adjustProbeExprNullableThroughOuterJoin(Plan leftChild,
@Override
public Boolean visitPhysicalHashJoin(PhysicalHashJoin<? extends Plan, ?
extends Plan> join,
PushDownContext ctx) {
+ // Scan conjuncts cannot fence ON expressions retained on the join,
including mark predicates.
+ if (hasEvaluationSensitiveExpressions(join)) {
Review Comment:
Fixed in 09d884ae0e. The earlier default-descent policy was too permissive,
so this revision changes the safety model instead of adding another operator
blacklist: unknown operators now stop traversal, and every traversable operator
explicitly establishes safety. PhysicalFilter checks all retained predicates
before descending; generators and expression-free cardinality assertions remain
closed.
The residual-WHERE, generator, and cardinality-assertion planner tests each
failed on the previous revision and now pass. Added SQL error/EXPLAIN cases for
the residual filter and generator. All 38 related FE tests, Checkstyle, and
Groovy compilation passed; full SQL execution remains pending CI.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/TopnFilterPushDownVisitor.java:
##########
@@ -108,6 +112,12 @@ public Boolean visitPhysicalProject(
return project.child().accept(this, ctxProjectProbeExpr);
}
+ private static boolean hasEvaluationSensitiveExpressions(Plan plan) {
+ return plan.getExpressions().stream().anyMatch(expression ->
+ expression.containsVolatileOrNoneMovableExpression()
+ ||
ExpressionUtils.containsUnsafeDateArithmetic(expression));
Review Comment:
Fixed in 09d884ae0e with a closed set of proven-total expressions.
Determinism or a monotonicity marker no longer grants permission to cross an
expression; unknown functions, casts, and UDFs are fenced by default. This
covers all three epoch conversions and future throwing families without having
to enumerate them. Safe ordinary filters/joins, literal-argument substring, and
zero-interval date arithmetic retain positive coverage.
The epoch-conversion test covers
from_second/from_millisecond/from_microsecond with a secondary sort key and
failed before the fix. Added negative-epoch SQL error/EXPLAIN cases. The
expanded four-suite FE run passed all 38 tests; the full SQL suite is pending
CI. The PR description now explains the conservative eligibility tradeoff.
--
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]
