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.

Sign up for GitHub

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

Jump to bottom

Progressive override (query planning / validation) #2902

Merged
merged 23 commits into from
Jan 19, 2024
Merged

Progressive override (query planning / validation) #2902

merged 23 commits into from
Jan 19, 2024

Conversation

trevor-scheer
Copy link
Member

@trevor-scheer trevor-scheer commented Jan 3, 2024 •
edited
Loading

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jan 3, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: f08a347

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

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 CodeSandbox CI
Copy link

codesandbox-ci bot commented Jan 3, 2024 •
edited
Loading

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.

@trevor-scheer trevor-scheer force-pushed the trevor/progressive-override-query-graphs branch from 66d4491 to f73f8a8 Compare January 6, 2024 17:18
Base automatically changed from trevor/progressive-override-composition to trevor/progressive-override January 8, 2024 20:01
@trevor-scheer trevor-scheer force-pushed the trevor/progressive-override-query-graphs branch 2 times, most recently from 0c55108 to bd1d73c Compare January 8, 2024 23:39
@lrlna lrlna mentioned this pull request Jan 11, 2024
Closed
sachindshinde
sachindshinde requested changes Jan 11, 2024
View reviewed changes
Copy link
Contributor

@sachindshinde sachindshinde left a comment

Choose a reason for hiding this comment

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

Sorry for the delays 🙇 , partial review here (still looking at composition validation logic).

internals-js/src/extractSubgraphsFromSupergraph.ts Outdated Show resolved Hide resolved
query-graphs-js/src/querygraph.ts Show resolved Hide resolved
query-graphs-js/src/querygraph.ts Outdated Show resolved Hide resolved
query-graphs-js/src/querygraph.ts Outdated Show resolved Hide resolved
query-planner-js/src/buildPlan.ts Outdated Show resolved Hide resolved
query-planner-js/src/buildPlan.ts Outdated Show resolved Hide resolved
query-graphs-js/src/querygraph.ts Outdated Show resolved Hide resolved
query-graphs-js/src/querygraph.ts Outdated Show resolved Hide resolved
query-graphs-js/src/querygraph.ts Outdated Show resolved Hide resolved
query-graphs-js/src/querygraph.ts Outdated Show resolved Hide resolved
sachindshinde
sachindshinde requested changes Jan 12, 2024
View reviewed changes
Copy link
Contributor

@sachindshinde sachindshinde left a comment

Choose a reason for hiding this comment

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

Here's the rest of the review, it's mainly about composition validation.

query-graphs-js/src/graphPath.ts Outdated Show resolved Hide resolved
composition-js/src/validate.ts Outdated Show resolved Hide resolved
composition-js/src/validate.ts Outdated Show resolved Hide resolved
composition-js/src/validate.ts Show resolved Hide resolved
@trevor-scheer trevor-scheer marked this pull request as ready for review January 17, 2024 16:55
@trevor-scheer trevor-scheer requested a review from a team as a January 17, 2024 16:55
@trevor-scheer trevor-scheer force-pushed the trevor/progressive-override-query-graphs branch 3 times, most recently from da5f596 to bf0e4e2 Compare January 18, 2024 20:15
@trevor-scheer trevor-scheer requested a review from a team as a January 18, 2024 20:21
@trevor-scheer trevor-scheer force-pushed the trevor/progressive-override-query-graphs branch from bf0e4e2 to 1f8c98c Compare January 18, 2024 20:22
@trevor-scheer trevor-scheer force-pushed the trevor/progressive-override-query-graphs branch from 6296652 to 041939e Compare January 19, 2024 00:28
sachindshinde
sachindshinde requested changes Jan 19, 2024
View reviewed changes
composition-js/src/validate.ts Outdated Show resolved Hide resolved
query-planner-js/src/buildPlan.ts Outdated Show resolved Hide resolved
query-graphs-js/src/querygraph.ts Outdated Show resolved Hide resolved
query-graphs-js/src/querygraph.ts Outdated Show resolved Hide resolved
query-graphs-js/src/graphPath.ts Outdated Show resolved Hide resolved
query-graphs-js/src/graphPath.ts Outdated Show resolved Hide resolved
query-graphs-js/src/graphPath.ts Outdated Show resolved Hide resolved
composition-js/src/validate.ts Outdated Show resolved Hide resolved
composition-js/src/validate.ts Outdated Show resolved Hide resolved
sachindshinde
sachindshinde approved these changes Jan 19, 2024
View reviewed changes
Copy link
Contributor

@sachindshinde sachindshinde left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing all the feedback! Just one minor nit below

query-graphs-js/src/querygraph.ts Outdated Show resolved Hide resolved
@trevor-scheer trevor-scheer changed the title WIP: progressive override (query planning) Progressive override (query planning / validation) Jan 19, 2024
@trevor-scheer trevor-scheer merged this pull request into trevor/progressive-override Jan 19, 2024
15 checks passed
@trevor-scheer trevor-scheer deleted the trevor/progressive-override-query-graphs branch January 19, 2024 20:28
@trevor-scheer trevor-scheer mentioned this pull request Jan 19, 2024
Merged
@trevor-scheer
Copy link
Member Author

@dariuszkuc I'm moving the backport-to-rust label over to #2911 which is the full PR for this feature (this one doesn't have the composition component).

trevor-scheer added a commit that referenced this pull request Jan 20, 2024
@trevor-scheer @sachindshinde
Progressive override (query planning / validation) (#2902)
29a2576 
Implement progressive override functionality in QP and validation logic

Co-authored-by: Sachin D. Shinde <[email protected]>
trevor-scheer added a commit that referenced this pull request Jan 20, 2024
@trevor-scheer @sachindshinde
Implement progressive @override (#2911)
6ae4294 
Add new optional `label` arg to `@override` which is a `String`. Capture
label in the supergraph via the new `@join__field` arg `overrideLabel`
so these values can be used during query graph creation and query
planning.

Reviewed in two separate PRs:
#2879
#2902

---------

Co-authored-by: Sachin D. Shinde <[email protected]>
@dariuszkuc dariuszkuc mentioned this pull request Sep 18, 2024
Merged
6 tasks
dariuszkuc added a commit to apollographql/router that referenced this pull request Sep 18, 2024
@dariuszkuc @goto-bus-stop
feat(federation): add support for progressive overrides (#6011)
efb752c 
Implement support for progressive `@override`s (apollographql/federation#2902).

Co-authored-by: Renée <[email protected]>
abernix pushed a commit to apollographql/router that referenced this pull request Sep 19, 2024
@dariuszkuc @goto-bus-stop @abernix
feat(federation): add support for progressive overrides (#6011)
78aef22 
Implement support for progressive `@override`s (apollographql/federation#2902).

Co-authored-by: Renée <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sachindshinde sachindshinde sachindshinde approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@trevor-scheer @sachindshinde @dariuszkuc