-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[fix](grouping sets) columns in grouping sets should not be in selectstmt‘s AggFunc #12313
Conversation
It seems that i got the exact same error a couple months ago, but the fix pr #10121 has not been merged into master yet. I think the recurrence rate of the current bug is quite high, and I hope that a short-term fix can be merged to master rather than waiting for the new query optimizer's implement. |
c8787fa
to
c669a97
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
fb21030
to
29ed956
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PR approved by at least one committer and no changes requested. |
if (item.getExpr() instanceof FunctionCallExpr && item.getExpr().fn instanceof AggregateFunction) { | ||
aggFnExprList.clear(); | ||
getAggregateFnExpr(item.getExpr(), aggFnExprList); | ||
if (!aggFnExprList.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is more clear and simple:
for (Expr aggFnExpr : aggFnExprList) { for (Expr expr : groupByClause.getGroupingExprs()) { .... } }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
private void getAggregateFnExpr(Expr expr, ArrayList<Expr> aggFnExprList) { | ||
if (expr instanceof FunctionCallExpr && expr.fn instanceof AggregateFunction) { | ||
aggFnExprList.add(expr); | ||
} else if (expr.getChildren() != null && expr.getChildren().size() > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove else if (expr.getChildren() != null && expr.getChildren().size() > 0) call for (Expr child : expr.getChildren()) { getAggregateFnExpr(child, aggFnExprList); }
directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reminder, I have modified it, but I have kept else if (expr.getChildren() != null)
, otherwise it may cause NullPointerException.
18edf8d
to
f48b95f
Compare
f48b95f
to
0a980fd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Proposed changes
Issue Number: close #12273
Fix grouping sets cause be core or return wrong results.
Problem summary
Describe your changes.
Checklist(Required)
Further comments
If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...