Over-aggressive optimisation can lead to generating non-optimal query plans #2623
Conversation
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.
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
🦋 Changeset detectedLatest commit: 982979e The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
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.
eaf5c0c to
c0ab4b1
Compare
query-graphs-js/src/graphPath.ts
Outdated
| * | ||
| * Please note that this method assumes that the 2 paths have the same root, and will fail if that's not the case. | ||
| */ | ||
| findBiggestCommonPrefixAndCountMainSubgraphJumpsAfterIt(that: GraphPath<TTrigger, RV, TNullEdge>): { |
There was a problem hiding this comment.
That's a mouthful. How about something like countSubgraphJumpsAfterCommon or something?
There was a problem hiding this comment.
Renamed to countSubgraphJumpsAfterLastCommonVertex to match the point below.
query-graphs-js/src/graphPath.ts
Outdated
| }; | ||
| } | ||
|
|
||
| private findBiggestCommonPrefixEndIndex(that: GraphPath<TTrigger, RV, TNullEdge>): { vertex: Vertex, index: number } { |
There was a problem hiding this comment.
Maybe findLastCommonVertexIndex instead?
There was a problem hiding this comment.
Yeah, make sense, renamed to findLastCommonVertex (we return both the vertex and index, so figure the Index part was unnecessary).
| // 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++) { |
There was a problem hiding this comment.
It's a little ugly to have a loop inside the previous loop that is really just a continuation of the first loop. Would be great if you could move this outside the enclosing for loop.
There was a problem hiding this comment.
I think that one might be a personal preference: as it is a continuation in only a specific case, moving that inner loop out means some sort of state tracking that I don't find more obvious.
The query planner has a number of heuristic it applies when computing possible paths to avoid generating options that can be proved early to never be better than other ones (as a basic example, if we have the same key between A -> B, A -> C and B -> C, and we're trying to go from A to C, then no point in considering doing A -> B -> C since we can do A -> C directly). Anyway, the point is that if we're not a bit smart in eliminating such "unnecessary" options, the number of final plans to evaluate can quickly explode (and make query planning long, up to the point where the planner stops degrading its output).
But one of the heuristic we're applying is unfortunately a bit too aggressive: it does remove options that are unnecessary, but was also removing options that should have been kept. The result is that the planner will sometime return plans that are not optimal (see the added test for an example).
This commit fix that problem by making the heuristic more precise and only remove the option that are safe to remove.
Side-note: the first commit of this PR is actually slightly unrelated to the description above, it is a small optimisation when generating plans. But I discovered both at the same time, and that optimisation was fairly simple, so took the liberty to include.