Skip to content
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.

Sign up for GitHub

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

Jump to bottom

opt(agg): reuse existing count(*) while generating stream plan #8197

Closed
Tracked by #8101
stdrc opened this issue Feb 27, 2023 · 6 comments · Fixed by #8242
Closed
Tracked by #8101

opt(agg): reuse existing count(*) while generating stream plan #8197

stdrc opened this issue Feb 27, 2023 · 6 comments · Fixed by #8242
Assignees
@stdrc
Labels
type/enhancement Improvements to existing implementation.
Milestone
release-0.18

Comments

@stdrc
Copy link
Member

stdrc commented Feb 27, 2023

No description provided.

@stdrc stdrc added the type/enhancement Improvements to existing implementation. label Feb 27, 2023
@stdrc stdrc self-assigned this Feb 27, 2023
@github-actions github-actions bot added this to the release-0.1.18 milestone Feb 27, 2023
This was referenced Feb 27, 2023
Closed
Merged
@jon-chuang
Copy link
Contributor

jon-chuang commented Feb 28, 2023 •
edited
Loading

Is this now fixed by #8200?

@stdrc
Copy link
Member Author

stdrc commented Feb 28, 2023

Is this now fixed by #8200?

No, the fix will be different from #8200. Will do this today😄

@stdrc stdrc mentioned this issue Feb 28, 2023
Merged
7 tasks
@stdrc stdrc linked a pull request Feb 28, 2023 that will close this issue
perf(stream,agg): reuse existing count(*) as internal row count agg call #8242
Merged
7 tasks
@jon-chuang
Copy link
Contributor

jon-chuang commented Mar 1, 2023 •
edited
Loading

Where is the performance improvement for reference? I guess anecdotal result is here: #7351 (comment)

@stdrc
Copy link
Member Author

stdrc commented Mar 2, 2023

Where is the performance improvement for reference? I guess anecdotal result is here: #7351 (comment)

Didn't do benchmark on this PR actually, cuz I think the performance increase is supposed to be minor (since it just remove one low-overhead agg call in some cases), a wild guess may be around 10% for nexmark q15/16/17? Anyway, I can do some bench later if it is necessary.😄

@jon-chuang
Copy link
Contributor

jon-chuang commented Mar 2, 2023 •
edited
Loading

Yes, I though I saw a graph about it showing throughput increase in the compute meeting but I couldn't find it after hunting a bit.

Anw, it's not necessary to create a bench, the anecdotal evidence is fine, as I wanted some justification for #8277

@stdrc
Copy link
Member Author

stdrc commented Mar 2, 2023

Yes, I though I saw a graph about it showing throughput increase in the compute meeting but I couldn't find it after hunting a bit.

Oh it was #8100

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@stdrc stdrc

Labels
type/enhancement Improvements to existing implementation.
Projects
None yet
Milestone
release-0.18
Development

Successfully merging a pull request may close this issue.

perf(stream,agg): reuse existing count(*) as internal row count agg call risingwavelabs/risingwave
2 participants
@stdrc @jon-chuang