Add a limit to the number of options for a selection by Geal · Pull Request #2880 · apollographql/federation

Geal · 2023-12-07T10:56:13Z

In some cases, the number of options generated for a selection can grow to a huge number, which results in a lot of time spent pruning options and creating plans. The new pathsLimit option will put a limit on that number of options

In some cases, the number of options generated for a selection can grow to a huge number, which results in a lot of time spent pruning options and creating plans. The new `pathsLimit` option will put a limit on that number of options

changeset-bot · 2023-12-07T10:56:17Z

🦋 Changeset detected

Latest commit: dcd405d

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

This PR includes changesets to release 7 packages

Name	Type
@apollo/query-planner	Patch
@apollo/gateway	Patch
@apollo/federation-internals	Patch
@apollo/composition	Patch
@apollo/query-graphs	Patch
@apollo/subgraph	Patch
apollo-federation-integration-testsuite	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

netlify · 2023-12-07T10:56:19Z

✅ Deploy Preview for apollo-federation-docs ready!

Name	Link
🔨 Latest commit	`dcd405d`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-federation-docs/deploys/6576cbc49664a300094ffb5d
😎 Deploy Preview	https://deploy-preview-2880--apollo-federation-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codesandbox-ci · 2023-12-07T10:56:58Z

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.

sachindshinde

Note that the PlanningParameters type has an argument named config which should have pathsLimit in it, so there's no need to manually pass through pathsLimit.

sachindshinde · 2023-12-07T15:53:31Z

query-planner-js/src/config.ts

+   *
+   * The default value is set to 10000
+   */
+  pathsLimit: number


You'll want to use ? here so that users won't be required to provide the property (and also to not break the API).

Suggested change

pathsLimit: number

pathsLimit?: number

Also could you place this property under the debug section for now? (Could potentially move it out later, but for now I'd rather we don't make stability guarantees around it.)

Geal · 2023-12-07T18:29:42Z

alright, looks like it's passing tests now

sachindshinde

Mainly mirroring some feedback from standup and some tweaks to wording in the doc string. (Also for some reason I don't understand, we only run prettier/formatting on the test files and not the main source files, so there's some manual whitespace fixes below as well.)

query-planner-js/src/config.ts

query-planner-js/src/buildPlan.ts

query-planner-js/src/config.ts

sachindshinde · 2023-12-08T20:08:25Z

Also you'll need to add a changeset file to this PR, there's instructions for what changesets are and how to add one in this comment #2880 (comment) .

Specifically you'll want to click the second link, which lets you add/commit the file via the GitHub UI.

Co-authored-by: Sachin D. Shinde <sachin@apollographql.com>

Geal · 2023-12-11T08:40:11Z

Also for some reason I don't understand, we only run prettier/formatting on the test files and not the main source files, so there's some manual whitespace fixes below as well

There may be some automatic formatter somewhere in my vscode. I don't remember setting it up for typescript though

Geal · 2023-12-11T08:51:51Z

alright, I think this can be merged now

sachindshinde

LGTM, merging it in

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @apollo/composition@2.6.2 ### Patch Changes - Updated dependencies \[[`7b5b836d15247c997712a47847f603aa5887312e`](7b5b836), [`74ca7dd617927a20d79b824851f7651ef3c40a4e`](74ca7dd), [`3f7392b84f8b626b248b59ce81f193d0f0272045`](3f7392b)]: - @apollo/federation-internals@2.6.2 - @apollo/query-graphs@2.6.2 ## @apollo/gateway@2.6.2 ### Patch Changes - Updated dependencies \[[`7b5b836d15247c997712a47847f603aa5887312e`](7b5b836), [`74ca7dd617927a20d79b824851f7651ef3c40a4e`](74ca7dd), [`ffe67dfbdb77d15dde2ab6dee66dba05c7b5c037`](ffe67df)]: - @apollo/federation-internals@2.6.2 - @apollo/query-planner@2.6.2 - @apollo/composition@2.6.2 ## @apollo/federation-internals@2.6.2 ### Patch Changes - Avoid `>=` comparison for `FeatureVersion` objects ([#2883](#2883)) - Fix query planning bug where `__typename` on interface object types in named fragments can cause query plan execution to fail. ([#2886](#2886)) ([#2886](#2886)) ## @apollo/query-graphs@2.6.2 ### Patch Changes - fix: handle directive conditions on fragments when building query graphs ([#2875](#2875)) This fix addresses issues with handling fragments when they specify directive conditions: - when exploding the types we were not propagating directive conditions - when processing fragment that specifies super type of an existing type and also specifies directive condition, we were incorrectly preserving the unnecessary type condition. This type condition was problematic as it could be referencing types from supergraph that were not available in the local schema. Instead, we now drop the redundant type condition and only preserve the directives (if specified). - Updated dependencies \[[`7b5b836d15247c997712a47847f603aa5887312e`](7b5b836), [`74ca7dd617927a20d79b824851f7651ef3c40a4e`](74ca7dd)]: - @apollo/federation-internals@2.6.2 ## @apollo/query-planner@2.6.2 ### Patch Changes - Add a limit to the number of options for a selection. In some cases, we will generate a lot of possible paths to access a field. There is a process to remove redundant paths, but when the list is too large, that process gets very expensive. To prevent that, we introduce an optional limit that will reject the query if too many paths are generated ([#2880](#2880)) - Updated dependencies \[[`7b5b836d15247c997712a47847f603aa5887312e`](7b5b836), [`74ca7dd617927a20d79b824851f7651ef3c40a4e`](74ca7dd), [`3f7392b84f8b626b248b59ce81f193d0f0272045`](3f7392b)]: - @apollo/federation-internals@2.6.2 - @apollo/query-graphs@2.6.2 ## @apollo/subgraph@2.6.2 ### Patch Changes - Updated dependencies \[[`7b5b836d15247c997712a47847f603aa5887312e`](7b5b836), [`74ca7dd617927a20d79b824851f7651ef3c40a4e`](74ca7dd)]: - @apollo/federation-internals@2.6.2 ## apollo-federation-integration-testsuite@2.6.2 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Add a limit to the number of options for a selection

2391a36

In some cases, the number of options generated for a selection can grow to a huge number, which results in a lot of time spent pruning options and creating plans. The new `pathsLimit` option will put a limit on that number of options

Geal requested a review from a team as a code owner December 7, 2023 10:56

sachindshinde requested changes Dec 7, 2023

View reviewed changes

Geal added 2 commits December 7, 2023 18:30

no need to explicitely pass optionsLimit as argument

e091c04

fix

2d1ac55

sachindshinde requested changes Dec 8, 2023

View reviewed changes

Apply suggestions from code review

c96a79d

Co-authored-by: Sachin D. Shinde <sachin@apollographql.com>

Geal added 2 commits December 11, 2023 09:43

Create changeset

1f14889

Merge branch 'main' into geal/options-limit

dcd405d

sachindshinde approved these changes Dec 11, 2023

View reviewed changes

sachindshinde merged commit ffe67df into main Dec 11, 2023

sachindshinde deleted the geal/options-limit branch December 11, 2023 22:10

github-actions bot mentioned this pull request Dec 11, 2023

release: on branch main #2876

Merged

dariuszkuc added the backport-to-rust label Jan 9, 2024

lrlna mentioned this pull request Jan 11, 2024

Ongoing log of issues and features to be backported apollographql/federation-next#133

Closed

lrlna mentioned this pull request Feb 6, 2024

backport add a limit to the number of options for a selection apollographql/federation-next#166

Closed

Conversation

Geal commented Dec 7, 2023

Uh oh!

changeset-bot bot commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

netlify bot commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for apollo-federation-docs ready!

Uh oh!

codesandbox-ci bot commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sachindshinde left a comment

Choose a reason for hiding this comment

Uh oh!

sachindshinde Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

Geal commented Dec 7, 2023

Uh oh!

sachindshinde left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sachindshinde commented Dec 8, 2023

Uh oh!

Geal commented Dec 11, 2023

Uh oh!

Geal commented Dec 11, 2023

Uh oh!

sachindshinde left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot bot commented Dec 7, 2023 •

edited

Loading

netlify bot commented Dec 7, 2023 •

edited

Loading

codesandbox-ci bot commented Dec 7, 2023 •

edited

Loading