yujun777 commented on code in PR #67919:
URL: https://github.com/apache/doris/pull/67919#discussion_r4060579885
##########
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] Apply this fence to eager aggregation
`PushDownAggregation`, `EagerAggRewriter`, and `PushDownAggContext` still
gate only on `containsVolatileExpression()`. After `ToBitmapWithCheck` is
marked `NoneMovableFunction`, an eager-aggregation plan with an empty probe and
`sum(bitmap_count(to_bitmap_with_check(r.a)))` can evaluate `r.a` on the build
side before the join and raise `InvalidArgument`, whereas the original upper
aggregate sees no rows. Please apply the shared non-movable/volatile check to
aggregate arguments, group keys, join conditions, and eager-aggregation
context, and add an empty-probe 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] Guard project-bearing TopN-through-join rewrites
`PushDownTopNThroughJoin` accepts project-bearing plans without checking the
project expressions. For `TopN -> Project(l.id, to_bitmap_with_check(l.a)) ->
LEFT JOIN`, pushing TopN into the preserved left child can discard an invalid
row before the project, suppressing an error that the original plan would
produce. Please add a project-mobility guard to both TopN-through-join branches
and cover this outer-join case.
##########
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] Guard projection movement across LIMIT
`PushDownProjectThroughLimit` unconditionally rewrites `Project(expr) ->
Limit` into `Limit -> Project(expr)`. For a project containing
`to_bitmap_with_check(a)`, this evaluates rows discarded by the original
LIMIT/OFFSET and can turn a successful query into `InvalidArgument`. Please
reject this transpose when the project contains a non-movable or otherwise
unsafe-on-additional-rows expression, with an OFFSET 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() {
+ return containsType(VolatileExpression.class,
NoneMovableFunction.class)
Review Comment:
[P1] Guard aggregate LIMIT/TopN project transposes
`LimitAggToTopNAgg` rewrites `LIMIT -> Project(to_bitmap_with_check(k)) ->
Aggregate` into `Project -> TopN -> Aggregate` without checking project
mobility. The synthesized TopN can discard an invalid group before the project,
suppressing the original `InvalidArgument`. `PullUpProjectBetweenTopNAndAgg`
has the same unchecked transpose. Please apply the
non-movable/unsafe-expression guard to these aggregate project paths and add a
grouped OFFSET regression.
--
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]
