github-actions[bot] commented on code in PR #67919:
URL: https://github.com/apache/doris/pull/67919#discussion_r4056586097
##########
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() {
+ return containsType(VolatileExpression.class,
NoneMovableFunction.class)
Review Comment:
[P1] Fence eager aggregation before moving marked inputs below joins
The eager-aggregation eligibility checks still use only
`containsVolatileExpression()`. With `eager_aggregation_mode=1`, consider:
```text
Aggregate(r.k, sum(bitmap_count(to_bitmap_with_check(r.a))))
InnerJoin(l.k = r.k)
runtime-empty L
R[(k=1,a=-1)]
```
The original upper aggregate receives no joined row, so it never evaluates
the invalid value. `PushDownAggregation` admits the `Sum`, assigns it to R from
its input slots, and `EagerAggRewriter` materializes a lower aggregate on R;
that evaluates `to_bitmap_with_check(-1)` before the join and raises
`InvalidArgument`. Please use this combined gate throughout the
eager-aggregation admission/context paths and add a forced-eager empty-probe
regression. This is distinct from the existing filter/hash/reorder threads
because the relocated expression is an aggregate argument.
##########
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() {
+ return containsType(VolatileExpression.class,
NoneMovableFunction.class)
+ && anyMatch(expr -> ((ExpressionTrait) expr).isVolatile() ||
expr instanceof NoneMovableFunction);
Review Comment:
[P1] Do not push marked projections below OFFSET
`PushDownProjectThroughLimit` unconditionally changes `Project(risky(a)) ->
Limit(1 OFFSET 1) -> child` into `Limit -> Project(risky(a)) -> child`. With an
ordered child whose first row is `a=-1` and second is `a=1`, the original Limit
discards the invalid row before the Project, while the rewrite evaluates it and
raises. The direct Sort shape can later normalize to TopN, but a non-Sort child
such as an already-TopN nested limit retains this transpose, including after
the late eliminate-stage registration. Please reject the rewrite when any
projected expression has this combined property and cover the OFFSET shape.
This is selected-expression movement, not the existing unused-project pruning
thread.
##########
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] Keep marked projects below aggregate LIMIT selection
`LimitAggToTopNAgg` rewrites `Limit(1 OFFSET 1) ->
Project(bitmap_count(to_bitmap_with_check(k))) -> Aggregate(group by k)` into
`Project -> TopN(k ASC,1 OFFSET 1) -> Aggregate`. With groups `k=-1` and `k=1`,
the original Project evaluates both group rows and raises on `-1`; the
synthesized TopN discards that group before the Project, so the rewritten query
succeeds. `push_topn_to_agg` is enabled by default, and
`PullUpProjectBetweenTopNAndAgg` has the same unchecked transpose when
compressed materialization is enabled. Please gate the project-bearing
aggregate transposes with the combined property and add this two-group OFFSET
regression.
##########
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] Preserve marked Project evaluation around TopN rewrites
For the deterministic plan
```text
TopN(order l.id ASC, limit 1)
Project(l.id, bitmap_count(to_bitmap_with_check(l.a)))
LeftJoin(L[(1,1),(2,-1)], R with one match per key)
```
the child Project evaluates both joined rows and errors on id 2.
`PushDownTopNThroughJoin` accepts this left-only project and inserts a child
TopN that removes id 2 before the Project; `PullUpProjectUnderTopN` can
independently move the whole Project above the outer TopN. Both suppress the
error and neither checks volatility or `NoneMovableFunction`. Please add a
shared project-mobility gate to the project-bearing Limit/TopN branches and
cover this ordered outer-join case. This is a TopN-specific row-domain change,
not the existing join-reorder/hash/NLJ paths.
--
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]
