Update buildQueryPlan for new Query Plan format #4448
Conversation
* ts errors gone
JakeDawkins
force-pushed
the
jake/planner-executor-updates
branch
from
9400fc2 to
51da4c6
Compare
JakeDawkins
force-pushed
the
jake/query-planner-types
branch
from
a773869 to
269a1c4
Compare
JakeDawkins
changed the title
There was a problem hiding this comment.
Thanks for putting this together! I'd appreciate @trevor-scheer's thoughts on this as well, but I've left a few comments within to which their answers might help me better review this.
I think the short-cuts are fair concessions to make, in the name of forward progress and stability as you noted. I would like us to be more explicit (in this PR, in comments in the code and possibly the commit messages) about why those are okay concessions. This could include otherwise unstated road-map intentions (including planned relocation of the execution functionality to Rust, etc.) and might also include open issues for (possible!) follow-up work in the event that those plans don't flesh out. (Otherwise, we might lose track!)
If we don't do that, I worry that a transformation of this kind feels like it could be classified as tech-debt and I think it might complicate an outside contributors/observers understanding the structures, which could be relevant — e.g. — when porting to another language.
(If we had a stated public specification or design document as to how this would look, this might change things, but for now all we have is this live-code to look at).
Do you think that makes sense?
packages/apollo-gateway/src/snapshotSerializers/queryPlanSerializer.ts
Outdated
Show resolved
Hide resolved
packages/apollo-gateway/src/snapshotSerializers/queryPlanSerializer.ts
Outdated
Show resolved
Hide resolved
packages/apollo-gateway/src/snapshotSerializers/astSerializer.ts
Outdated
Show resolved
Hide resolved
packages/apollo-gateway/src/snapshotSerializers/astSerializer.ts
Outdated
Show resolved
Hide resolved
packages/apollo-gateway/src/snapshotSerializers/queryPlanSerializer.ts
Outdated
Show resolved
Hide resolved
packages/apollo-gateway/src/snapshotSerializers/queryPlanSerializer.ts
Outdated
Show resolved
Hide resolved
packages/apollo-gateway/src/snapshotSerializers/queryPlanSerializer.ts
Outdated
Show resolved
Hide resolved
packages/apollo-gateway/src/snapshotSerializers/queryPlanSerializer.ts
Outdated
Show resolved
Hide resolved
- add new flattenEntitiesfield in place of old function which did a fuzzy check of a
query's signature to eliminate the _entities field from a pretty-printed query plan.
- this new function should be much safer, and harder to fool by accident
- the rename also makes more sense for what it's doing -- it flattens an _entities operation
two levels—getting rid of the `query {` and `_entities` and flattens other query definitions
just to remove `query {`
- fix added newlines from the pretty-printed query plan serializer
- format files
- add plenty of comments and descriptions around utility functions
JakeDawkins
force-pushed
the
jake/query-planner-types
branch
from
3868752 to
93d1962
Compare
There was a problem hiding this comment.
Couple notes within, but LGTM to merge into #4410 and pickup over there! Thanks, @JakeDawkins!
This PR builds off of #4410
That PR defines the query plan types and the queryPlanExecutor, and this PR updates the query plan builder.
Rather than refactoring the internals of every utility function in the query planner, I focused on two places: the fetch nodes and the selection sets. These two places are pretty much the leaves of the query plan and the only places that were actually changed by the type updates.
The important things to look at in this PR are the changes to the
buildQueryPlanfile and the serializers. I decided to update the serializers to work with the new query plan format, so we still have a pretty-printable interface for the query plan, and so snapshots across the codebase wouldn't have to all be manually reviewed 😅. The serializer updated added a newline after fragments, but I figure that's a small price to pay.Note: Since I didn't update the entire query planner internals, and added some mappers at the end, I do expect this query planner to be marginally slower than the existing one. It was either this, or undergo a massive refactor. I think this is an okay tradeoff, since a refactor of that complexity (which I was previously working on) could introduce some pretty nasty untested bugs or edge cases we hadn't previously seen.
todo: