-
Notifications
You must be signed in to change notification settings - Fork 0
perf(sql): speed up GetForecastAtTimestamp via DISTINCT #102
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
Conversation
Benchmark ResultsBenchmark resultsBenchmark vs base branch |
|
Althought benchmark is not working, previous PR have So its gone down form 23.67m to 7.5m |
| LIMIT ( | ||
| SELECT COUNT(*) FROM loc.geometries | ||
| WHERE geometry_uuid = ANY(sqlc.arg(geometry_uuids)::UUID []) | ||
| ) |
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.
I wante to just do LIMIT length(sqlc.arg(geometry_uuids)::UUID []), but it didnt seem to work
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.
I'm not sure this LIMIT does anything other than doing an uneccessary count procedure - you're already getting DISTINCT on the geometry_uuid and filtering to asked-for geometry UUIDs in the WHERE clause. Is there any noticable difference in the speed if you remove it?
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.
ah, let me check
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.
no noticiable limit, so Ill remove this
Worth noting that there's a bug in the main benchmark - it hasn't updated for a while. The current speed of the GetForecastAtTimestamp is more like ~20ms. I need to look into this! |
Co-authored-by: devsjc <[email protected]>
Contribution Checklist
make lintwith your changes locally?make testwith your changes locally?Warning
PRs may be closed if all the above boxes are not checked.
Changes in this Pull Request
Reduces the amount of forecasts that are collected in the ForecastAtTimestamp query