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

Optim local value types #2449

Merged
merged 3 commits into from
Mar 15, 2023
Merged

Conversation

pcmanus
Copy link
Contributor

@pcmanus pcmanus commented Mar 10, 2023

Often times, value types only references either "leaf" types or other value types, but no entity type or root types. When a sub-part of a query arrives to such a type, then we know that the rest of the subselection is going to also be part of whichever fetch we're currently building. We can use that knowledge to save work.

This is what this commit does. First, it pre-computes when building the query planner which types have no reachable entity or root type starting from them. Then, when computing query plans, it checks when such types is reached, and when it is, it short-cuts the building of the GraphPaths and PathTrees for the remaining sub-selections.

When subgraphs have a large number of value types (especially some deeply nested ones), this can measurably speed up query plan generation. This is particularly true when one federate either a single subgraph or one subgraph is much large, which is a corner cases, but can happen in the process of migrating an existing monolith to federation).

@changeset-bot
Copy link

changeset-bot bot commented Mar 10, 2023

🦋 Changeset detected

Latest commit: 90ad424

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
apollo-federation-integration-testsuite Patch
@apollo/query-planner Patch
@apollo/query-graphs Patch
@apollo/composition Patch
@apollo/federation-internals Patch
@apollo/subgraph Patch
@apollo/gateway Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 10, 2023

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.

@pcmanus
Copy link
Contributor Author

pcmanus commented Mar 10, 2023

I'll note that at the time of this writing, this PR is based on top of #2387, and this because there is intersections of code and I didn't wanted to incur rebase pains. Besides, the cases where this PR helps are also helped by #2387, so it makes sense to me to review that earlier PR first.

internals-js/package.json Outdated Show resolved Hide resolved
private attachDirective(directive: Directive<any>): Directive<T> {
// if the directive is not attached, we can assume we're fine just attaching it to use. Otherwise, we're "copying" it.
const toAdd = directive.isAttached()
? new Directive(directive.name, directive.arguments())
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if this would be a little cleaner as a clone method on Directive.

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've kept as-is because I think it's the only place we do this for now and I was lacking inspiration on naming (it's not fully cloning since it doesn't keep the parent in particular). We can extract into a method later if this is done in other places.

query-graphs-js/src/pathTree.ts Outdated Show resolved Hide resolved
Sylvain Lebresne added 2 commits March 15, 2023 14:32
Often times, value types only references either "leaf" types or other
value types, but no entity type or root types. When a sub-part of a
query arrives to such a type, then we know that the rest of the
subselection is going to also be part of whichever fetch we're currently
building. We can use that knowledge to save work.

This is what this commit does. First, it pre-computes when building
the query planner which types have no reachable entity or root type
starting from them. Then, when computing query plans, it checks when
such types is reached, and when it is, it short-cuts the building of
the `GraphPath`s and `PathTree`s for the remaining sub-selections.

When subgraphs have a large number of value types (especially some
deeply nested ones), this can measurably speed up query plan
generation. This is particularly true when one federate either a single
subgraph or one subgraph is much large, which is a corner cases, but
can happen in the process of migrating an existin monolith to
federation).
@pcmanus pcmanus merged commit cab383b into apollographql:next Mar 15, 2023
@clenfest clenfest added this to the 2.4 milestone Mar 16, 2023
pcmanus pushed a commit to pcmanus/federation that referenced this pull request Mar 21, 2023
Code was added in apollographql#2449 which "optimize" selections by removing
unecessary inline fragments and branches that can't be satisfied (when
we cross type conditions so that the intersection of types leading here
is empty).

Unfortunately that code misses some cases of when a branch is
impossible, which leads to trying to build a selection set that is
invalid and this in turn triggers an assertion error.
pcmanus pushed a commit to pcmanus/federation that referenced this pull request Mar 21, 2023
Code was added in apollographql#2449 which "optimize" selections by removing
unecessary inline fragments and branches that can't be satisfied (when
we cross type conditions so that the intersection of types leading here
is empty).

Unfortunately that code misses some cases of when a branch is
impossible, which leads to trying to build a selection set that is
invalid and this in turn triggers an assertion error.
pcmanus pushed a commit to pcmanus/federation that referenced this pull request Mar 23, 2023
Code was added in apollographql#2449 which "optimize" selections by removing
unecessary inline fragments and branches that can't be satisfied (when
we cross type conditions so that the intersection of types leading here
is empty).

Unfortunately that code misses some cases of when a branch is
impossible, which leads to trying to build a selection set that is
invalid and this in turn triggers an assertion error.
pcmanus pushed a commit that referenced this pull request Mar 23, 2023
…2486)

Code was added in #2449 which "optimize" selections by removing
unecessary inline fragments and branches that can't be satisfied (when
we cross type conditions so that the intersection of types leading here
is empty).

Unfortunately that code misses some cases of when a branch is
impossible, which leads to trying to build a selection set that is
invalid and this in turn triggers an assertion error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants