feat: Query planner skips fetches when possible (based on @skip and @include usages
#1113
Conversation
trevor-scheer
requested review from
martijnwalraven,
prasek,
abernix and
glasser
It's reasonable for the query planner to make use of top-level @Skip and @include usages in order to determine whether it can omit/ignore a FetchNode altogether and prevent an unnecessary call to a service when possible. This commit implements this behavior. FetchNodes are appended with the sum of all "inclusion conditions" of the top level fields. Each condition has a possible skip and include value, and those values can be either a literal (true/false) or a variable. During execution, the executor will reduce all of the conditions based on literal and variable values into a single "skip or include" decision, and will either execute or ignore that FetchNode. A number of edge cases were considered before and during the implementation of this improvement. The most notable: 1. Using skip and include on the same selection (always skip unless skip: false AND include: true). This adheres to the spec 2. FragmentSpread and InlineFragment cases. This behavior is partially implemented. There are test cases (skipped) to demonstrate both ideal behavior and existing limitations of this improvement. Resolving these limitations requires some other, unrelated changes to the QP impl. details which I consider to be out of scope for this change. These cases are also less likely to occur and I find the current changeset to be large enough to defer solving this edge case for a later time if/when it's deemed useful/necessary.
trevor-scheer
force-pushed
the
trevor/conditional-fetches
branch
from
b525f2c to
cb5aeb2
Compare
|
Looks like I broke something in Rust land w.r.t. the query planner. cc @o0Ignition0o 👋 |
|
It looks like the overflow happens during json deserialization, I wonder how deep the structure is or if there is some circular dependency somewhere. Let me fiddle with it real quick :) |
@o0Ignition0o I'm reviewing this right now, but I suspect this is due to the addition of |
There was a problem hiding this comment.
I had a hard time following some of the logic in here, but I realize it's a complicated problem to solve. Let's talk this over during our call today, but here are some thoughts.
As far as I can see, the current approach will break down for more complicated cases (when multiple @skip/@include directives are applied to the same selection for example, because we only take the last encountered one of each kind into account). If the only consequence of that is that we perform an unnecessary fetch that would be ok, but I think it could also lead to us skipping a fetch that should not be skipped.
We should probably adjust the way we encode the conditions we store on FetchNode. Rather than a structure that only allows for a single skip and include, it might make sense to store an array of conditions per selection. For inline fragments or fragment spreads, I think we could either flatten the conditions (propagating them while walking the AST, and duplicating them for every field) or store conditions in a tree.
| if (conditionalType === 'boolean') { | ||
| return conditionals[conditional] as boolean; | ||
| } else if (conditionalType === 'string') { | ||
| return variables[conditionals[conditional] as string] as boolean; |
There was a problem hiding this comment.
The diff is hard to read so it's worth noting that I caught an error here (was casting as string when I should've been casting as boolean.
It's reasonable for the query planner to make use of top-level
@skipand@includeusages in order to determine whether it can omit/ignore aFetchNodealtogether and prevent an unnecessary call to a service when possible.This PR implements this behavior.
FetchNodes are appended with the sum of all "inclusion conditions" of the top level fields. Each condition has a possibleskipandincludevalue, and those values can be either aBooleanliteral (true/false) or a variable.During execution, the executor will reduce all of the conditions based on literal and variable values into a single "skip or include" result, and will execute or ignore that
FetchNodebased on the result.A number of edge cases were considered before and during the implementation of this improvement. The most notable:
@skipand@includeon the same selection (always skip unlessskip: falseANDinclude: true). This adheres to the spec.FragmentSpreadandInlineFragmentcases. This behavior is partially implemented. There are test cases (skipped) to demonstrate both ideal behavior and existing limitations of this improvement. Resolving these limitations requires some other, unrelated changes to the QP impl. details which I consider to be out of scope for this change. These cases are also less likely to occur and I find the current changeset to be large enough to defer solving this edge case for a later time if/when it's deemed useful/necessary.