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

Improve merging of groups during @require handling #1732

Merged
merged 1 commit into from Apr 21, 2022

Conversation

pcmanus
Copy link
Contributor

@pcmanus pcmanus commented Apr 14, 2022

Query planning handles @requires by, when it needs to handle a field
with requires, computing a sub-plan to gather the requirements
(so-called "fetch groups") and then checking if those groups can be
merged into earlier groups to avoid needless fetches.

The test to check if we could merge into earlier groups was a bit
conservative in 2 small ways:

  1. as part of the process, we were looking if we had created a useless
    fetch group, one where all the selections were already parts of
    its inputs. It was however possible to have a selection not strictly
    included in the inputs, but just because of a few additional
    __typename. And those shouldn't prevent optimizing since we can
    always move those to the parent of whichever group were were trying
    to optimize away (__typename is essentially always resolved by all
    subgraphs).
  2. further down, we were only merging groups where "merge path" were
    matching, but this was limiting merging when some nesting was
    involved. The patch fixes this by always merging but ensuring we
    properly concatenate path as needed to handle nesting.

Fixes #1725

Query planning handles `@requires` by, when it needs to handle a field
with requires, computing a sub-plan to gather the requirements
(so-called "fetch groups") and then checking if those groups can be
merged into earlier groups to avoid needless fetches.

The test to check if we could merge into earlier groups was a bit
conservative in 2 small ways:
1. as part of the process, we were looking if we had created a useless
   fetch group, one where all the selections were already parts of
   its inputs. It was however possible to have a selection not strictly
   included in the inputs, but just because of a few additional
   `__typename`. And those shouldn't prevent optimizing since we can
   always move those to the parent of whichever group were were trying
   to optimize away (`__typename` is essentially always resolved by all
   subgraphs).
2. further down, we were only merging groups where "merge path" were
   matching, but this was limiting merging when some nesting was
   involved. The patch fixes this by always merging but ensuring we
   properly concatenate path as needed to handle nesting.

Fixes apollographql#1725
@netlify
Copy link

netlify bot commented Apr 14, 2022

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 501c5fb

@codesandbox-ci
Copy link

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 clenfest merged commit 633dbc1 into apollographql:main Apr 21, 2022
benweatherman added a commit that referenced this pull request Apr 25, 2022
# [2.0.2-alpha.0] - 2022-04-22

## 🚀 Features

- Improve fed1 schema support during composition [PR #1735](#1735)
- Add gateway version to schema extensions [PR #1751](#1751)

## 🐛 Fixes

- Improve merging of groups during `@require` handling in query planning [PR #1732](#1732)
- Move `__resolveReference` resolvers on to `extensions` [PR #1746](#1746)
- Honor directive imports when directive name is spec name [PR #1720](#1720)

## 🛠 Maintenance

- Improved renovate bot auto-updates for 0.x packages [PR #1736](#1736) and [PR #1730](#1730)
- Add missing `@apollo/federation-internals` dependency to gateway [PR #1721](#1721)
- Migrate to `@apollo/utils` packages for `createSHA` and `isNodeLike` [PR #1765](#1765)

## 📚 Documentation

- Roadmap updates! [PR #1717](#1717)
- Clarify separation of concerns in the intro docs [PR #1753](#1753)
- Update intro example for fed2 [PR #1741](#1741)
- Improve error doc generation, add hints generation, add scrolling style to too-large error tables [PR #1740](#1740)
- Update `supergraphSDL` to be a string when creating an `ApolloGateway` [PR #1744](#1744)
- Federation subgraph library compatibility updates [PR #1718](#1744)
@benweatherman benweatherman mentioned this pull request May 3, 2022
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.

Fed2 query planning regression with @requires
2 participants