Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

feat: Intra-subgraph logic of federated query graph creation #84

Merged
merged 2 commits into from
Nov 14, 2023

Conversation

sachindshinde
Copy link
Contributor

@sachindshinde sachindshinde commented Nov 8, 2023

This PR implements the intra-subgraph logic of federated query graph creation. That is, it implements the business logic that is performed for each individual subgraph when creating a federated query graph. The inter-subraph logic will be a separate PR, to keep PRs smaller. Tests will be added in follow-up PRs.

Some notes:

  • Regarding porting, while the JS codebase creates a separate query graph for each subgraph and then copies its nodes/edges over to the federated one, the Rust code in this PR starts out with a single query graph (the federated one), and each builder adds its nodes/edges to that one. This allows us to avoid copying all the subgraph query graphs (and makes the porting/working with petgraph a bit easier).
  • We introduce a few convenience functions for FederationSchema/positions in this PR. We use those in the QueryGraph logic, but I've also refactored extract_subgraphs_from_supergraph() logic to use them, which has simplified some functions/removed lifetimes (also some bugfixes).
  • This PR is stacked on top of feat: Handle reserved names/meta-fields properly #80 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code defines the query graph data structure and some accessors, corresponding to the analogous parts of querygraph.ts in the JS codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is for building the query graph, corresponding to the relevant parts of querygraph.ts. Note that this PR only implements federated query graph creation (API query graph creation is only used by composition and will be deferred to later), and this PR specifically only implements the intra-subgraph part of federated query graph creation.

@sachindshinde sachindshinde force-pushed the sachin/create-query-graphs-part-1 branch from 18a0e15 to ac9b07e Compare November 9, 2023 22:34
@sachindshinde sachindshinde force-pushed the sachin/create-query-graphs-part-2 branch from a551912 to 32ed206 Compare November 9, 2023 22:41
sachindshinde added a commit that referenced this pull request Nov 10, 2023
…0-beta.6` (#85)

This PR exists mainly to unblock the rebasing of #80 and #84, as they'll have merge conflicts with the updated code (and I'd like to update the code for those PRs today). After I've finished rebasing those two PRs, will file a PR for 2nd half (probably later today).
@sachindshinde sachindshinde force-pushed the sachin/create-query-graphs-part-1 branch 2 times, most recently from fd550dd to 18466fc Compare November 13, 2023 14:01
@sachindshinde sachindshinde force-pushed the sachin/create-query-graphs-part-2 branch from eea28c6 to 824ff74 Compare November 13, 2023 15:43
@goto-bus-stop
Copy link
Member

goto-bus-stop commented Nov 14, 2023

Tests will be added in follow-up PRs.

Are there tests for just the intra-subgraph logic? I think we should aim to get those in as soon as possible, the later we do it the more risk there is of some small discrepancy somewhere in the codebase being super hard to debug.

Base automatically changed from sachin/create-query-graphs-part-1 to main November 14, 2023 16:37
@sachindshinde sachindshinde force-pushed the sachin/create-query-graphs-part-2 branch from 824ff74 to 2fa7923 Compare November 14, 2023 17:15
@sachindshinde
Copy link
Contributor Author

@goto-bus-stop
Most of the tests are for query planning itself (there's a few tests for query graphs as a whole), there aren't tests directly for intra-subgraph logic from what I can see. It's a division I made primarily to break up a large PR into smaller PRs.

Agreed we should get tests in sooner rather than later; the main incentive behind getting code in is to unblock other work sooner.

@sachindshinde sachindshinde merged commit 2ee4fe9 into main Nov 14, 2023
@sachindshinde sachindshinde deleted the sachin/create-query-graphs-part-2 branch November 14, 2023 17:36
SimonSapin pushed a commit to apollographql/router that referenced this pull request May 3, 2024
…0-beta.6` (apollographql/federation-next#85)

This PR exists mainly to unblock the rebasing of apollographql/federation-next#80 and apollographql/federation-next#84, as they'll have merge conflicts with the updated code (and I'd like to update the code for those PRs today). After I've finished rebasing those two PRs, will file a PR for 2nd half (probably later today).
SimonSapin pushed a commit to apollographql/router that referenced this pull request May 3, 2024
…raphql/federation-next#84)

This PR implements the intra-subgraph logic of federated query graph creation. That is, it implements the business logic that is performed for each individual subgraph when creating a federated query graph. The inter-subraph logic will be a separate PR, to keep PRs smaller. Tests will be added in follow-up PRs.

Some notes:
- Regarding porting, while the JS codebase creates a separate query graph for each subgraph and then copies its nodes/edges over to the federated one, the Rust code in this PR starts out with a single query graph (the federated one), and each builder adds its nodes/edges to that one. This allows us to avoid copying all the subgraph query graphs (and makes the porting/working with `petgraph` a bit easier).
- We introduce a few convenience functions for `FederationSchema`/positions in this PR. We use those in the `QueryGraph` logic, but I've also refactored `extract_subgraphs_from_supergraph()` logic to use them, which has simplified some functions/removed lifetimes (also some bugfixes).
- This PR is stacked on top of apollographql/federation-next#80 .
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants