github-actions[bot] commented on code in PR #64293:
URL: https://github.com/apache/doris/pull/64293#discussion_r3384674430
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/CountLiteralRewrite.java:
##########
@@ -107,10 +118,71 @@ private boolean isCountLiteral(AggregateFunction aggFunc)
{
&& aggFunc.child(0).isLiteral();
}
+ private boolean isCountConstantExpression(AggregateFunction aggFunc) {
+ if (aggFunc.isDistinct()
+ || !(aggFunc instanceof Count)
+ || aggFunc.children().size() != 1) {
+ return false;
+ }
+ Expression arg = aggFunc.child(0);
+ return !arg.isLiteral()
+ && arg.foldable()
+ && !arg.containsNondeterministic()
+ && !arg.containsVolatileExpression()
+ && arg.getInputSlots().isEmpty()
+ && !arg.containsType(Sleep.class)
+ && !arg.containsType(AggregateFunction.class,
SubqueryExpr.class,
+ TableGeneratingFunction.class, WindowExpression.class);
+ }
+
private Expression rewrite(Count count) {
if (count.child(0).isNullLiteral()) {
return new BigIntLiteral(0);
}
+ if (!count.child(0).isLiteral()) {
+ Expression countStar = new Count();
+ Expression constExpr = deferConstantEvaluation(count.child(0));
+ Expression ifConstantExprIsNull = new If(new IsNull(constExpr),
new BigIntLiteral(0), countStar);
+ return new If(new EqualTo(countStar, new BigIntLiteral(0)), new
BigIntLiteral(0), ifConstantExprIsNull);
+ }
return new Count();
}
+
+ /*
+ * count(const_expr) should not evaluate const_expr when the aggregate
input is empty.
+ * The outer count(*) = 0 guard handles that at execution time, but BE
opens constant
+ * expression trees eagerly. Add an unreachable count(*) dependency inside
each constant
+ * subtree so that it is no longer treated as a fragment-local constant
before the guard
+ * can short-circuit it.
+ */
+ private Expression deferConstantEvaluation(Expression expression) {
+ if (expression.isLiteral()) {
+ return expression;
+ }
+ List<Expression> children = expression.children();
+ List<Expression> newChildren =
Lists.newArrayListWithCapacity(children.size());
+ boolean changed = false;
+ for (Expression child : children) {
+ Expression newChild = deferConstantEvaluation(child);
+ newChildren.add(newChild);
+ changed |= newChild != child;
+ }
+
+ Expression rewritten = changed ? expression.withChildren(newChildren)
: expression;
Review Comment:
This wrapper makes the first child of every constant subtree depend on
`count(*)`, so that child is no longer a constant expression. That breaks
scalar functions whose BE implementation requires a specific argument to remain
constant. For example, `SELECT count(date_trunc('day', date '2024-01-02')) FROM
count_constant_rewrite_test` is a valid query and should return the row count,
but this rewrite turns the time-unit argument into `if(count(*) >= 0, 'day',
NULL)`. The BE `date_trunc` implementation checks
`context->is_col_constant(DateArgIsFirst ? 1 : 0)` in `open()` and returns
`date_trunc function of time unit argument must be constant.` Please either
exclude expressions with constant-argument contracts from this rewrite or defer
evaluation in a way that preserves those required constant arguments, and add
coverage for such a function. This is distinct from the existing `sleep()` and
empty-input threads because the failure is caused by the anti-folding wrapper
changing a require
d constant argument into a non-constant one.
--
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]
