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.

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

Merged
merged 8 commits into from
Oct 30, 2024

Conversation

gtarpenning
Copy link
Member

@gtarpenning gtarpenning commented Oct 25, 2024

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

@circle-job-mirror
Copy link

circle-job-mirror bot commented Oct 25, 2024

@gtarpenning gtarpenning marked this pull request as ready for review October 25, 2024 18:47
@gtarpenning gtarpenning requested a review from a team as a code owner October 25, 2024 18:47
@@ -6,12 +6,27 @@
from weave.trace_server.orm import ParamBuilder


def assert_sql(cq: CallsQuery, exp_queries, exp_params):
pb = ParamBuilder("pb")
Copy link
Collaborator

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):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are qr, qe?

Copy link
Member Author

@gtarpenning gtarpenning Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

queryRequest, queryExpected

queryReal?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk

@gtarpenning gtarpenning enabled auto-merge (squash) October 30, 2024 17:47
@gtarpenning gtarpenning merged commit 7a8a673 into master Oct 30, 2024
116 checks passed
@gtarpenning gtarpenning deleted the griffin/split-calls-query-when-heavy branch October 30, 2024 18:37
@github-actions github-actions bot locked and limited conversation to collaborators Oct 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants