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

fix(query): Do not execute filters if there are no source uids (#7962… #8452

Merged
merged 2 commits into from
Nov 29, 2022

Conversation

all-seeing-code
Copy link
Contributor

@all-seeing-code all-seeing-code commented Nov 23, 2022

No need to execute filter subgraph if there are no source UIDs.

(cherry picked from commit 849b587)

Description

The original PR adds a fail fast methodology, where it doesn't expand on a filter subgraph if the srcUid is nil. The current cherry-pick PR brings that change in and adds a test case that scenario.

Test

TestFilterWithNoSrcUid

@CLAassistant
Copy link

CLAassistant commented Nov 23, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ anurags92
❌ ahsanbarkati
You have signed the CLA already but the status is still pending? Let us recheck it.

@all-seeing-code all-seeing-code added the cherry-pick-22.0.2 Label for all cherry-picks for 22.0.2 Release label Nov 23, 2022
@coveralls
Copy link

coveralls commented Nov 23, 2022

Coverage Status

Coverage decreased (-0.001%) to 37.19% when pulling cce6ab7 on cherry-pick-7962 into 9a2427e on main.

@@ -230,25 +230,27 @@ type Function struct {
// SubGraph is the way to represent data. It contains both the request parameters and the response.
// Once generated, this can then be encoded to other client convenient formats, like GraphQL / JSON.
// SubGraphs are recursively nested. Each SubGraph contain the following:
// * SrcUIDS: A list of UIDs that were sent to this query. If this subgraph is a child graph, then the
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we changing anything significant in the comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we are not. But these are go fmt changes which can be safely accepted.

@@ -2210,6 +2212,10 @@ func ProcessGraph(ctx context.Context, sg, parent *SubGraph, rch chan error) {
}

filter.SrcUIDs = sg.DestUIDs
if len(filter.SrcUIDs.Uids) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an existing test for this scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a test TestFilterWithNoSrcUid here.

ahsanbarkati and others added 2 commits November 28, 2022 17:33
#7969)

No need to execute filter subgraph if there are no source UIDs.

(cherry picked from commit 849b587)
Copy link

@nswamy nswamy left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for bringing in the change and adding the test.

@all-seeing-code all-seeing-code merged commit 141944a into main Nov 29, 2022
@all-seeing-code all-seeing-code deleted the cherry-pick-7962 branch November 29, 2022 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-22.0.2 Label for all cherry-picks for 22.0.2 Release
Development

Successfully merging this pull request may close these issues.

7 participants