Optim local value types #2449
Merged
Optim local value types #2449
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 90ad424 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
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 |
|
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. |
clenfest
approved these changes
Mar 14, 2023
internals-js/src/definitions.ts
Outdated
| 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()) |
There was a problem hiding this comment.
Wondering if this would be a little cleaner as a clone method on Directive.
There was a problem hiding this comment.
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.
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
force-pushed
the
optim-local-value-types
branch
from
67ed4d5 to
e69f90a
Compare
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 andPathTrees 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).