-
Notifications
You must be signed in to change notification settings - Fork 67
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
perf(weave): split calls query when using a heavy condition #2779
Conversation
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=a6b55a606bcde7883323fb2fa7c2ed90ca572a2d |
@@ -6,12 +6,27 @@ | |||
from weave.trace_server.orm import ParamBuilder | |||
|
|||
|
|||
def assert_sql(cq: CallsQuery, exp_queries, exp_params): | |||
pb = ParamBuilder("pb") |
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.
every time I see this I think protobuf 🙃
queries = cq.as_sql(pb) | ||
params = pb.get_params() | ||
|
||
for qr, qe in zip(queries, exp_queries): |
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.
what are qr, qe
?
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.
queryRequest, queryExpected
queryReal?
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.
idk
Description
WB-21659
This PR adds functionality to the CallsQuery builder to optionally generate two queries instead of one when conditions are right. If the query has a heavy filter or order by, we can't push down into the subquery, which causes a heinous join and bad performance. To remedy this we split the query into two parts, resolving the set of all possible call IDs, then adding that as a param to the param builder, then making a second query. This dance is managed by a helper function:
_construct_query_str_maybe_do_pre_query
which always returns a valid sql query string. If we need to make a pre-query to get the IDs it happens in the helper, and returns the formatted next query.Testing
added test