-
Notifications
You must be signed in to change notification settings - Fork 242
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
Change query planner API to avoid double parsing and schema building #628
Change query planner API to avoid double parsing and schema building #628
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaict, this is a relatively simple cleanup and it looks good to me.
85935b6
to
a124bf3
Compare
We originally kept the same API we used for the wasm query planner, but that relied on passing in strings for the schema and operation. Since the query planner is now implemented in TypeScript again, we can avoid double parsing and schema building by passing in a composed schema and a parsed operation.
32be0ec
to
a6ce5d7
Compare
query-planner-js/src/index.ts
Outdated
buildOperationContext(planner_ptr.composedSchema, parse(query)), | ||
options, | ||
); | ||
// TODO: We should change the API to avoid confusion, because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #632
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
…tionHeaders * upstream/main: (77 commits) Update CHANGELOGs for release Release Change query planner API to avoid double parsing and schema building (apollographql#628) Update documentation around delivery endpoint option (apollographql#630) Change `buildComposedSchema` test to check for specific directives and types Add `query-planner-js` to `tsconfig.test.json` Release chore(ci): Bump `cache-cargo` seed value. chore(ci): Remove special-case install of `gnu-tar` on `macos` GH Actions. chore(ci): Never try to restore the cache without the `cache-name` for npm chore(ci): Expand upon cache restore keys for Cargo restorations chore(ci): Be explicit about cache restore keys Release Update uplink usage to latest url and update query/generated types (apollographql#626) Release Update changelogs (optionally) REVERT ME: cachebuster Update the thing that TS didn't catch for me :sadface: Rename Endpoint -> Graph and serviceName -> graphName Avoid unnecessary check for undefined ...
…tionHeaders * upstream/main: (77 commits) Update CHANGELOGs for release Release Change query planner API to avoid double parsing and schema building (apollographql#628) Update documentation around delivery endpoint option (apollographql#630) Change `buildComposedSchema` test to check for specific directives and types Add `query-planner-js` to `tsconfig.test.json` Release chore(ci): Bump `cache-cargo` seed value. chore(ci): Remove special-case install of `gnu-tar` on `macos` GH Actions. chore(ci): Never try to restore the cache without the `cache-name` for npm chore(ci): Expand upon cache restore keys for Cargo restorations chore(ci): Be explicit about cache restore keys Release Update uplink usage to latest url and update query/generated types (apollographql#626) Release Update changelogs (optionally) REVERT ME: cachebuster Update the thing that TS didn't catch for me :sadface: Rename Endpoint -> Graph and serviceName -> graphName Avoid unnecessary check for undefined ...
We originally kept the same API we used for the wasm query planner, but that relied on passing in strings for the schema and operation. Since the query planner is now implemented in TypeScript again, we can avoid double parsing and schema building by passing in a composed schema and a parsed operation.
There are remaining inefficiencies in the gateway workflows that we'll want to address in follow-up PRs. In particular, the gateway still calls
buildComposedSchema
twice for every update, because it extracts the service list to perform health checks in a separate function. We also parse the supergraph SDL multiple times due to the way functions are structured. Solving this will likely require a rethinking of the way these workflows are composed, and the possible introduction of additional objects (see #580).