Fix fragment reuse in subgraph fetches #1911
Conversation
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
| // cases we won't even reach that point either because there is no fragment, or none will have been | ||
| // optimized away so we'll exit above). We can optimize later if this show up in profiling though. | ||
| // | ||
| // Also note `toDeoptimize` will always contains the unused fragments, which will allow `expandFragments` |
There was a problem hiding this comment.
Note to reviewers: this comment is the reason of the change in this method. Essentially, it removes the prior minUsagesToOptimize > condition so expandFragments can "clean up" unused fragments in all cases.
| @@ -648,10 +676,6 @@ export class SelectionSet extends Freezable<SelectionSet> { | |||
| } | |||
|
|
|||
| collectUsedFragmentNames(collector: Map<string, number>) { | |||
| if (!this.fragments) { | |||
There was a problem hiding this comment.
Note to reviewers: that condition (and the same below) was currently incorrect because this.fragments is not always set at all the levels of a SelectionSet. We could probably change this and using this.fragments is set at all the levels, but that's a bit easy to get wrong so it feels that not relying on that was less fragile.
pcmanus
force-pushed
the
1894-fragments-fix
branch
from
481220b
to
e600149
Compare
|
Pushed additional commits to introduce a query planner/gateway option to opt-out of reusing fragments in subgraph queries. Both because that de-risk things a bit in the sense that this give a opt-out if this patch goes wrong for someone for some reason, but also because in general this option can be a tad of a tradeoff in that even if it can saves you from potentially super large subgraph queries, it also takes some cycles to look if fragments can be reused or not. As of those commit, I still left the reusing of fragments as the default, and the option is to opt-out, because I believe query plans are usually well cached so that time-to-compute-a-plan is not absolutely critical for most users (and so, by default, avoiding the risk of subgraph query size getting out of hand feels like a better default that optimising every ms out of the plan computation). I get that it's a judgment and I'm not beyond discussing it. |
There was a problem hiding this comment.
Using some real-world queries and schemas, I see a pretty drastic slowdown of query planning. I know we've talked previously that we don't necessarily wanna optimize for "query plans per second" or anything like that, but I wonder if we should do some more digging on this before we're ready to merge. I don't have a good sense for "yeah this will slow down query planning, but it's still better because the request size to subgraphs is much smaller" so thought I'd start the convo.
EDIT: I have some granular results for said queries & schemas; would it be helpful to look through those results to see if there are some common patterns? Maybe it would also be helpful to look at query plan size?
EDIT EDIT: We went through the results! Coupled with the other performance changes that have landed recently, these fragment reuse changes really improve things. Thanks for the diligence working on this!
The query planner has code originally meant to reuse fragments from the user query in subgraph fetches when it made sense. Unfortunately that code wasn't triggered at all due to the query planner not properly capturing those fragments. This ticket fixes this issue, adds more testing and improves a few related issues. Fixes apollographql#1894
pcmanus
force-pushed
the
1894-fragments-fix
branch
from
e600149
to
9c9a2d6
Compare
The code from apollographql#1911 ensures that named fragments from the user query are reused in subgraph query "when appropriate", but there were some code path where a fragment that was reused (correctly) could end up be "re-expanded" (due to an existing method not handling spreads properly). The resulting subgraph query ended up with the fragment definition existing but never being used, which is invalid in graphQL. This patch ensures that a reused fragment is not mistakenly re-expanded by the code (and thus avoids a query with unused fragments). Fixes apollographql#2092
The code from apollographql#1911 ensures that named fragments from the user query are reused in subgraph query "when appropriate", but there were some code path where a fragment that was reused (correctly) could end up be "re-expanded" (due to an existing method not handling spreads properly). The resulting subgraph query ended up with the fragment definition existing but never being used, which is invalid in graphQL. This patch ensures that a reused fragment is not mistakenly re-expanded by the code (and thus avoids a query with unused fragments). Fixes apollographql#2092
The code from #1911 ensures that named fragments from the user query are reused in subgraph query "when appropriate", but there were some code path where a fragment that was reused (correctly) could end up be "re-expanded" (due to an existing method not handling spreads properly). The resulting subgraph query ended up with the fragment definition existing but never being used, which is invalid in graphQL. This patch ensures that a reused fragment is not mistakenly re-expanded by the code (and thus avoids a query with unused fragments). Fixes #2092
The patch for apollographql#1911 introduced an issue where directives applied to a fragment spread were lost when the named fragments was reused. This commit fixes it by ensuring we properly copy those directives. Fixes apollographql#2124
The query planner has code originally meant to reuse fragments from the
user query in subgraph fetches when it made sense. Unfortunately that
code wasn't triggered at all due to the query planner not properly
capturing those fragments. This ticket fixes this issue, adds more
testing and improves a few related issues.
Fixes #1894