github-actions[bot] commented on code in PR #67919:
URL: https://github.com/apache/doris/pull/67919#discussion_r4046707720
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/ToBitmapWithCheck.java:
##########
@@ -37,7 +38,7 @@
* ScalarFunction 'to_bitmap_with_check'. This class is generated by
GenerateFunction.
*/
public class ToBitmapWithCheck extends ScalarFunction
- implements UnaryExpression, ExplicitlyCastableSignature,
AlwaysNotNullable {
+ implements UnaryExpression, ExplicitlyCastableSignature,
AlwaysNotNullable, NoneMovableFunction {
Review Comment:
[P1] Fence standalone mark hash-key selection too
This marker is still ignored by the standalone mark-join path. A projected
nullable `bitmap_count(to_bitmap_with_check(l.a)) IN (SELECT r.b FROM
runtime_empty_nullable_r)` leaves an `EqualPredicate` in `markJoinConjuncts`;
`PushDownExpressionsInHashCondition` uses the volatile-only classifier, and
`JoinUtils.shouldNestedLoopJoin` independently treats any mark equality as
hash-eligible. FE then installs the expression as a hash key, so BE evaluates
`to_bitmap_with_check(-1)` even with an empty build, while the nested-loop mark
path returns false without evaluating the conjunct. Fixing the ordinary-ON
classifier from thread 4045976810 is therefore not sufficient: please apply one
combined non-movable/volatile eligibility check to both mark-key projection and
hash-vs-NLJ selection, and add an empty nullable-build regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/ExpressionTrait.java:
##########
@@ -114,4 +114,9 @@ default boolean isVolatile() {
default boolean containsVolatileExpression() {
return containsType(VolatileExpression.class) && anyMatch(expr ->
((ExpressionTrait) expr).isVolatile());
}
+
+ default boolean containsVolatileOrNoneMovableExpression() {
Review Comment:
[P1] Fence constant-UNION project folding with this helper
`PushProjectIntoUnion` still checks only volatility. For `SELECT IF(flag,
bitmap_count(x), 0) FROM (SELECT FALSE flag, to_bitmap_with_check(-1) x UNION
ALL SELECT TRUE, to_bitmap_with_check(1)) u`, the original childless Union
materializes every constant expression, so the negative call raises. The rule
instead substitutes each row into the project and folds the first `IF(FALSE,
risky, 0)` to `0`, suppressing that evaluation and returning rows. Here `x` is
referenced, so this is distinct from the unused-alias thread and from filter
cloning/CASE extraction. Please reject this push when a referenced constant row
contains the combined non-movable/volatile property, and add this conditional
constant-UNION regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownFilterThroughSetOperation.java:
##########
@@ -87,7 +87,7 @@ public Rule build() {
pushableConjuncts = new LinkedHashSet<>();
Set<Expression> kept = new LinkedHashSet<>();
for (Expression c : origFilter.getConjuncts()) {
- if (c.containsVolatileExpression()) {
+ if (c.containsVolatileOrNoneMovableExpression()) {
Review Comment:
[P1] Fence marker-bearing rows on the UNION ALL fast path
This combined predicate check is bypassed for `UNION ALL`, but a harmless
pushed predicate can still erase evaluation of a non-movable row output. For
`SELECT bitmap_count(x) FROM (SELECT FALSE flag, to_bitmap_with_check(-1) x
UNION ALL SELECT TRUE, to_bitmap_with_check(1)) u WHERE flag`, the original
Union eagerly materializes both constant rows and the negative call raises. The
fast path substitutes `flag`, folds the first row's filter to false, and
`EliminateFilter` drops that whole marker-bearing row before evaluation. The
marker is not in the predicate, so the existing set-filter thread's fence does
not cover this, and guarding only `PushProjectIntoUnion` would not help. Please
reject this row-eliminating push when an affected constant/one-row output
contains the combined property, with a regression for this witness.
--
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]
