Over-aggressive optimisation can lead to generating non-optimal query plans (#2623)

When there is multiple valid choices for multiple paths of a query, the
number of theoretically possible ends up a cartesian product so it
explodes fairly quickly, and generating too many plans can be very
costly. To manage this in general, we generate the possible plans
incrementally and in an order that is likely to yield "good" plans
early, and use the cost of previously computed plan to possibly
cut early further computation.

The code doing this was however not optimal as we weren't using that
potential of early exit as aggressively as we could. This commit
fixes that.

Further, when building plans, after we've computed all the possible options to
get to a particular leaf of the query being plan, we're applying an
heuristic that checks if an option is guaranteed to be always better
than another one (and remove the latter one if so), which the goal
of reducing the number of actual full plan we need to evaluate.

But the implementation of that heuristic was incorrect: while it was
correctly removing options that we did meant to remove, it was also
sometimes removing options that really should be kept.

This resulted in planning sometimes not produce the most optimal
possible plan.

This commit fixes that heuristic to only exclude what can be safely
excluded.

Author: Sylvain Lebresne

.changeset/slimy-geckos-invite.md

@@ -0,0 +1,7 @@
+---
+"@apollo/query-planner": patch
+"@apollo/query-graphs": patch
+---
+
+Fix query planner heuristic that could lead to ignoring some valid option and yielding a non-optimal query plan.
+  

query-graphs-js/src/graphPath.ts

@@ -232,22 +232,131 @@ export class GraphPath<TTrigger, RV extends Vertex = Vertex, TNullEdge extends n
     return this.props.edgeIndexes.length;
   }
 
-  subgraphJumps(): number {
+  /**
+   * That method first look for the biggest common prefix to `this` and `that` (assuming that both path are build as choices
+   * of the same "query path"), and the count how many subgraph jumps each of the path has after said prefix.
+   *
+   * Note that this method always return someting but the biggest common prefix considered might well be empty. 
+   *
+   * Please note that this method assumes that the 2 paths have the same root, and will fail if that's not the case.
+   */
+  countSubgraphJumpsAfterLastCommonVertex(that: GraphPath<TTrigger, RV, TNullEdge>): {
+    thisJumps: number,
+    thatJumps: number
+  } {
+    const { vertex, index } = this.findLastCommonVertex(that);
+    return {
+      thisJumps: this.subgraphJumpsAtIdx(vertex, index),
+      thatJumps: that.subgraphJumpsAtIdx(vertex, index),
+    };
+  }
+
+  private findLastCommonVertex(that: GraphPath<TTrigger, RV, TNullEdge>): { vertex: Vertex, index: number } {
+    let vertex: Vertex = this.root;
+    assert(that.root === vertex, () => `Expected both path to start on the same root, but 'this' has root ${vertex} while 'that' has ${that.root}`);
+
+    const minSize = Math.min(this.size, that.size);
+    let index = 0;
+    for (; index < minSize; index++) {
+      const thisEdge = this.edgeAt(index, vertex);
+      const thatEdge = that.edgeAt(index, vertex);
+      if (thisEdge !== thatEdge) {
+        break;
+      }
+      if (thisEdge) {
+        vertex = thisEdge.tail;
+      }
+    }
+    return { vertex, index};
+  }
+
+  private subgraphJumpsAtIdx(vertex: Vertex, index: number): number {
     let jumps = 0;
-    let v: Vertex = this.root;
-    for (let i = 0; i < this.size; i++) {
+    let v: Vertex = vertex;
+    for (let i = index; i < this.size; i++) {
       const edge = this.edgeAt(i, v);
       if (!edge) {
         continue;
       }
-      if (edge.transition.kind === 'KeyResolution' || edge.transition.kind === 'RootTypeResolution') {
+      if (edge.changesSubgraph()) {
         ++jumps;
       }
       v = edge.tail;
     }
     return jumps;
   }
 
+  subgraphJumps(): number {
+    return this.subgraphJumpsAtIdx(this.root, 0);
+  }
+
+  isEquivalentSaveForTypeExplosionTo(that: GraphPath<TTrigger, RV, TNullEdge>): boolean {
+    // We're looking a the specific case were both path are basically equivalent except
+    // for a single step of type-explosion, so if either the paths don't start and end in the
+    // same vertex, or if `other` is not exactly 1 more step than `this`, we're done.
+    if (this.root !== that.root || this.tail !== that.tail || this.size !== that.size - 1) {
+      return false;
+    }
+
+    // If that's true, then we get to our comparison.
+    let thisV: Vertex = this.root;
+    let thatV: Vertex = that.root;
+    for (let i = 0; i < this.size; i++) {
+      let thisEdge = this.edgeAt(i, thisV);
+      let thatEdge = that.edgeAt(i, thatV);
+      if (thisEdge !== thatEdge) {
+        // First difference. If it's not a "type-explosion", that is `that` is a cast from an
+        // interface to one of the implementation, then we're not in the case we're looking for.
+        if (!thisEdge || !thatEdge || !isInterfaceType(thatV.type) || thatEdge.transition.kind !== 'DownCast') {
+          return false;
+        }
+        thatEdge = that.edgeAt(i+1, thatEdge.tail);
+        if (!thatEdge) {
+          return false;
+        }
+        thisV = thisEdge.tail;
+        thatV = thatEdge.tail;
+
+        // At that point, we want both path to take the "same" key, but because one is starting
+        // from the interface while the other one from an implementation, they won't be technically
+        // the "same" edge object. So we check that both are key, to the same subgraph and type,
+        // and with the same condition.
+        if (thisEdge.transition.kind !== 'KeyResolution'
+          || thatEdge.transition.kind !== 'KeyResolution'
+          || thisEdge.tail.source !== thatEdge.tail.source
+          || thisV !== thatV
+          || !thisEdge.conditions!.equals(thatEdge.conditions!)
+        ) {
+          return false;
+        }
+
+        // So far, so good. `thisV` and `thatV` are positioned on the vertex after which the path
+        // must be equal again. So check that it's true, and if it is, we're good.
+        // Note that for `this`, the last edge we looked at was `i`, so the next is `i+1`. And
+        // for `that`, we've skipped over one more edge, so need to use `j+1`.
+        for (let j = i + 1; j < this.size; j++) {
+          thisEdge = this.edgeAt(j, thisV);
+          thatEdge = that.edgeAt(j+1, thatV);
+          if (thisEdge !== thatEdge) {
+            return false;
+          }
+          if (thisEdge) {
+            thisV = thisEdge.tail;
+            thatV = thatEdge!.tail;
+          }
+        }
+        return true;
+      }
+      if (thisEdge) {
+        thisV = thisEdge.tail;
+        thatV = thatEdge!.tail;
+      }
+    }
+    // If we get here, both path are actually exactly the same. So technically there is not additional
+    // type explosion, but they are equivalent and we can return `true`.
+    return true;
+  }
+
   [Symbol.iterator](): PathIterator<TTrigger, TNullEdge> {
     const path = this;
     return {
@@ -1814,6 +1923,35 @@ export function advanceSimultaneousPathsWithOperation<V extends Vertex>(
           // is unsatisfiable. But as we've only taken type preserving transitions, we cannot get an empty results at this point if we haven't
           // had one when testing direct transitions above (in which case we have exited the method early).
           assert(pathWithOperation.length > 0, () => `Unexpected empty options after non-collecting path ${pathWithNonCollecting} for ${operation}`);
+
+          // There is a special case we can deal with now. Namely, suppose we have a case where a query
+          // is reaching an interface I in a subgraph S1, we query some field of that interface x, and
+          // say that x is provided in subgraph S2 but by an @interfaceObject for I.
+          // As we look for direct options for I.x in S1 initially, as we didn't found `x`, we will have tried
+          // to type-explode I (in say implementation A and B). And in some case doing so is necessary, but
+          // it may also lead for the type-exploding option to look like:
+          //  [
+          //    I(S1) -[... on A]-> A(S1) -[key]-> I(S2) -[x] -> Int(S2),
+          //    I(S1) -[... on B]-> B(S1) -[key]-> I(S2) -[x] -> Int(S2),
+          //  ]
+          // But as we look at indirect option now (still from I in S1), we will note that we can also
+          // do:
+          //    I(S1) -[key]-> I(S2) -[x] -> Int(S2),
+          // And while both options are technically valid, the new one really subsume the first one: there
+          // is no point in type-exploding to take a key to the same exact subgraph if using the key
+          // on the interface directly works.
+          //
+          // So here, we look for that case and remove any type-exploding option that the new path
+          // render unecessary.
+          // Do note that we only make that check when the new option is a single-path option, because
+          // this gets kind of complicated otherwise.
+          if (pathWithNonCollecting.tailIsInterfaceObject()) {
+            for (const indirectOption of pathWithOperation) {
+              if (indirectOption.length === 1) {
+                options = options.filter((opt) => !opt.every((p) => indirectOption[0].isEquivalentSaveForTypeExplosionTo(p)));
+              }
+            }
+          }
           options = options.concat(pathWithOperation);
         }
         debug.groupEnd();
@@ -1853,6 +1991,7 @@ export function advanceSimultaneousPathsWithOperation<V extends Vertex>(
   return createLazyOptions(allOptions, subgraphSimultaneousPaths, updatedContext);
 }
 
+
 export function createInitialOptions<V extends Vertex>(
   initialPath: OpGraphPath<V>,
   initialContext: PathContext,

query-planner-js/src/__tests__/buildPlan.test.ts

@@ -4423,8 +4423,7 @@ describe('__typename handling', () => {
           t: T @shareable
         }
 
-        type T @key(fields: "id") {
-          id: ID!
+        type T {
           x: Int
         }
       `
@@ -4438,7 +4437,7 @@ describe('__typename handling', () => {
           t: T @shareable
         }
 
-        type T @key(fields: "id") {
+        type T {
           id: ID!
           y: Int
         }
@@ -6170,3 +6169,98 @@ describe('`debug.maxEvaluatedPlans` configuration', () => {
   });
 });
 
+test('correctly generate plan built from some non-individually optimal branch options', () => {
+  // The idea of this test is that the query has 2 leaf fields, `t.x` and `t.y`, whose
+  // options are:
+  //  1. `t.x`:
+  //     a. S1(get t.x)
+  //     b. S2(get t.id) -> S3(get t.x using key id)
+  //  2. `t.y`:
+  //     a. S2(get t.id) -> S3(get t.y using key id)
+  //
+  // And the idea is that "individually", for just `t.x`, getting it all in `S1` using option a.,
+  // but for the whole plan, using option b. is actually better since it avoid querying `S1`
+  // entirely (and `S2`/`S2` have to be queried anyway).
+  //
+  // Anyway, this test make sure we do correctly generate the plan using 1.b and 2.a, and do
+  // not ignore 1.b in favor of 1.a in particular (which a bug did at one point).
+
+  const subgraph1 = {
+    name: 'Subgraph1',
+    typeDefs: gql`
+      type Query {
+        t: T @shareable
+      }
+
+      type T {
+        x: Int @shareable
+      }
+    `
+  }
+
+  const subgraph2 = {
+    name: 'Subgraph2',
+    typeDefs: gql`
+      type Query {
+        t: T @shareable
+      }
+
+      type T @key(fields: "id") {
+        id: ID!
+      }
+    `
+  }
+
+  const subgraph3 = {
+    name: 'Subgraph3',
+    typeDefs: gql`
+      type T @key(fields: "id") {
+        id: ID!
+        x: Int @shareable
+        y: Int
+      }
+    `
+  }
+
+  const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2, subgraph3);
+  const operation = operationFromDocument(api, gql`
+    {
+      t {
+        x
+        y
+      }
+    }
+  `);
+
+  const plan = queryPlanner.buildQueryPlan(operation);
+  expect(plan).toMatchInlineSnapshot(`
+   QueryPlan {
+     Sequence {
+       Fetch(service: "Subgraph2") {
+         {
+           t {
+             __typename
+             id
+           }
+         }
+       },
+       Flatten(path: "t") {
+         Fetch(service: "Subgraph3") {
+           {
+             ... on T {
+               __typename
+               id
+             }
+           } =>
+           {
+             ... on T {
+               y
+               x
+             }
+           }
+         },
+       },
+     },
+   }
+  `);
+});