Adding an option to Auto-refragment the QueryPlan#2463
Adding an option to Auto-refragment the QueryPlan#2463AneethAnand wants to merge 2 commits intoapollographql:nextfrom
Conversation
|
|
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. |
AneethAnand
force-pushed
the
aneeth/autoFragmentizeQP
branch
3 times, most recently
from
758b70f to
136abd2
Compare
… the subgraph request if it could not reuse the existing one
AneethAnand
force-pushed
the
aneeth/autoFragmentizeQP
branch
3 times, most recently
from
b1f0243 to
1939213
Compare
…to fragment but also enable re-using fragments the partially matched the selection set.
AneethAnand
force-pushed
the
aneeth/autoFragmentizeQP
branch
from
1939213 to
d801312
Compare
pcmanus
left a comment
pcmanus
left a comment
There was a problem hiding this comment.
Left some comments, but I think the first and main thing is that this will need to be rebased substantially due to #2497 having been merge. That later PR refactor the optimise method somewhat and actually already handle some of the changes in this PR (which will thus not be necessary).
I do feel a bit bad because that need for rebasing is on me, so please do let me know if you want me to help with that rebase and I can do it.
| `); | ||
| }); | ||
|
|
||
| test('Reuse fragment if possible else auto re-fragment', () => { |
There was a problem hiding this comment.
The code on main should be a lot more able to reuse fragments, which might change the output of some of those tests. But as the goal of the option added here is to create fragments that don't exists, maybe the test can start from operation that have repetition and no fragments, and show that some fragments are indeed created.
| } | ||
|
|
||
| optimize(fragments?: NamedFragments, minUsagesToOptimize: number = 2): Operation { | ||
| optimize(fragments?: NamedFragments, options: OptimizeOptions = {}, minUsagesToOptimize: number = 2): Operation { |
There was a problem hiding this comment.
This ends up having 2 version of minUsagesToOptimize, one in OptimizeOptions and one as separate argument. I suggest removing the argument and only use the one in the options, but maybe preserve the existing default.
|
|
||
| export interface OptimizeOptions { | ||
| minUsagesToOptimize?: number | ||
| autoFragmetize?: boolean |
There was a problem hiding this comment.
Nit: s/autoFragmetize/autoFragmentize/
| optimize(fragments?: NamedFragments, options: OptimizeOptions = {}, minUsagesToOptimize: number = 2): Operation { | ||
| assert(minUsagesToOptimize >= 1, `Expected 'minUsagesToOptimize' to be at least 1, but got ${minUsagesToOptimize}`) | ||
| if (!fragments || fragments.isEmpty()) { | ||
| if (!fragments || (!options.autoFragmetize && fragments.isEmpty())){ |
There was a problem hiding this comment.
In practice, fragments will be undefined if there operation being query planner as no. Since the new option is all about creating fragments, I'm not sure it make sense to exit if !fragments. Instead, I think this condition should look like:
if (!options.autoFragmentize && (!fragments || fragments.isEmpty())) {
return this;
}If does imply that a new empty NamedFragments object must be created, but I actually think that we should always create a new NamedFragments for autoFragmentize because the method will modify that object and I think we should avoid modifying the input object as that might be unexpected by callers (and could easily lead to hard-to-find issues).
| export interface OptimizeOptions { | ||
| minUsagesToOptimize?: number | ||
| autoFragmetize?: boolean | ||
| schema?: Schema |
There was a problem hiding this comment.
I don't think we need/should have the schema in the options. It's used when creating new fragment inside the Selection, but Selection has access to the underlying schema with this.element.schema(). And it's a bit weird to have it as an option here when Operation.optimize always ignore it if it is passed as option.
|
|
||
| const usages = new Map<string, number>(); | ||
| optimizedSelection.collectUsedFragmentNames(usages); | ||
| // fragments. |
There was a problem hiding this comment.
Nit: I'd remove this comment :).
| optimize(fragments?: NamedFragments): SelectionSet { | ||
| if (!fragments || fragments.isEmpty()) { | ||
| optimize(fragments?: NamedFragments, options: OptimizeOptions = {}): SelectionSet { | ||
| if (!fragments || (!options.autoFragmetize && fragments.isEmpty())) { |
There was a problem hiding this comment.
Same remark that on Operation regarding !fragments. In that case however, it's probably stranger to create a new NamedFragments objects. But we actually never pass undefined to this particular method (we do for Operation.optimize however), so I might suggest just making fragments mandatory here.
| if (optimizedSelection && optimizedSelection.equals(candidate.selectionSet)) { | ||
| const fragmentSelection = new FragmentSpreadSelection(fieldBaseType, fragments, candidate.name); | ||
| return new FieldSelection(this.field, selectionSetOf(fieldBaseType, fragmentSelection)); | ||
| } else if(options?.autoFragmetize && optimizedSelection && optimizedSelection.contains(candidate.selectionSet)) { |
There was a problem hiding this comment.
Some similar contains check has been added on main, so the patch here will only need the later part where a fragment is optimistically generated if the remaining selection is not "trivial". But I'm afraid this will require a bit of rebase, sorry.
| // If we don't see the same pattern the final pass of optimizer will re-expand it. | ||
| const schema = options?.schema; | ||
| if(options?.autoFragmetize && schema && optimizedSelection.selections().length > 1) { | ||
| const hash = createHash('sha256').update(optimizedSelection.toString()).digest('hex'); |
There was a problem hiding this comment.
I believe the only goal of that hash is to generate a unique name for the created fragment. If so, I would suggest using a simple uuid here (using the uuidv1() method already imported in this file). This is going to be substantially cheaper to generate (optimizedSelection.toString() is not exactly cheap) and will lead to overall smaller identifiers (more readable, and I guess makes the overall query smaller).
| * Whether the query planner should try to automatically fragment queries that | ||
| * are expanded during query to minimize the query size sent to subgraphs. | ||
| */ | ||
| experimental_autoFragmentQuery?: boolean, |
There was a problem hiding this comment.
I'd prefer using something like:
experimental?: {
autoFragmentSubgraphQueries?: boolean
}so that we can later have more experimental option in a slightly cleaner way. If you rebase on main, you'll see that we added a similar debug section for option that exists for debugging/testing purpose. In fact, an argument could maybe be made for adding this new option to that existing debug category (but I'm ok with an experimental section too as that's not exactly the same notion than debug).
As show above, I might also suggest renaming "autoFragmentQuery" into "autoFragmentSubgraphQueries" to make it immediately clear which queries we're talking about (mostly, that this is not about the "input" query).
|
Wait, is this superseded by generateQueryFragments (PR #2958)? |
|
Yes it was. Superseded by #2958 |
4 participants
PR related to: #2457