-
Notifications
You must be signed in to change notification settings - Fork 248
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
[query] Fix BAD loop deoptimization with StreamAgg #12995
Conversation
CHANGELOG: Fixed bug causing poor performance and memory leaks for Matrix.annotate_rows aggregations
bump, has a bug |
bump |
@patrick-schultz can you adopt this PR? |
Yeah, I got it |
@tpoterba I think I understand what was happening. I think NestingDepth wasn't handling aggregation right at all. AggLet was getting its nesting depth from the eval scope, which got compared to the nesting depth of the ref's agg scope. So even if both were at the same level inside an aggregation, we weren't forwarding. We were also ignoring StreamAggScan. Could you check if this looks right to you? |
I think there's another separate bug. The problem was that an ir fragment containing a StreamAgg was getting compiled, and it still had an AggLet after LowerArrayAggsToRunAggs. It's now getting inlined, but this should still work even if it isn't inlined. Maybe LowerArrayAggsToRunAggs needs to convert that to a normal Let. |
@patrick-schultz can you confirm that you anticipated this to succeed now and it just needed to be retried or something? Just a bit concerning to see a merge after two weeks without changes. |
Yeah, I've been continually retrying. It was passing non-service tests when I approved two weeks ago. |
CHANGELOG: Fixed bug causing poor performance and memory leaks for Matrix.annotate_rows aggregations