Add new generateQueryFragments option to query planner config

[trevor-scheer]

If enabled, the query planner will extract inline fragments into fragment definitions before sending queries to subgraphs. This can significantly reduce the size of the query sent to subgraphs, but may increase the time it takes to plan the query.

This is a reimplementation of #2893 that applies transformations on the selection set instead of AST.

Credit to @samuelAndalon for suggesting the new approach. Resolves #2892

[netlify]

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	e8560f8

[changeset-bot]

🦋 Changeset detected

Latest commit: e8560f8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
apollo-federation-integration-testsuite	Minor
@apollo/query-planner	Minor
@apollo/federation-internals	Minor
@apollo/gateway	Minor
@apollo/composition	Minor
@apollo/query-graphs	Minor
@apollo/subgraph	Minor

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

[codesandbox-ci]

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.

[sachindshinde]

Spoke to Trevor about the PR, had some suggestions there but will summarize them here:

• The approach we're taking where "every eligible inline fragment gets replaced with a fragment spread" is okay for now, given that (1) this is feature flagged, and (2) there are some users who just flat-out need the compression above all else, and soon. But it's a terrible choice for readability, and there's extra overhead from the unnecessary fragment indirection (for processing and validation), so I'm going to write out a design for a separate algorithm some time next week. We can take that as a follow-up.
• Using the stringified selection set doesn't quite work, since we don't care about the order of various things (selections, directives, arguments, object value fields). We could implement an actual .hashCode() function, but I think we're too time-constrained here for that. The suggestion I've made there is to instead use the length of the stringified selection set as a hash function (as it's unaffected by order changes). Collisions will mostly happen at low sizes, but those should be fast to compare via .equals() anyway, so it's fine in the short-term.
  • This means changing the map of stringified selection sets to named fragment definitions to instead become a map of hash codes (numbers) to lists of pairs, where each pair consists of a SelectionSet object, along with the named fragment definition. Lookup involves computing the hash of the SelectionSet, and if an entry is found, iterating through the pairs in the list, comparing the SelectionSet to the ones in the pairs via .equals(). If a pair is equal, then the SelectionSet is "in" the map, and the corresponding named fragment definition is the value. If no pair is equal (or no entry was found), then the SelectionSet is not "in" the map, and we'd append to that list a new pair with that SelectionSet and the new named fragment definition (or if no entry was found, add an entry with the hash code and a new list containing just that pair).

[dariuszkuc]

The suggestion I've made there is to instead use the length of the stringified selection set as a hash function (as it's unaffected by order changes)

I think "stringifying" selection set is going to be rather slow for computing the keys. Why not just go with something simple as selection.key() + selection.selectionSet.selections().length instead? We are going to be comparing the selection sets on hits anyway, so first usingselection.key() we will limit to the same type condition + directives and then we would just need to quickly compare the size of selection set size to achieve the same thing.

If you are going to be pointing to an array of NamedFragmentDefinition then we still need to account for the additional index in order to generate unique fragment name -> you could simply incorporate that index with the key and avoid nested loop.

[sachindshinde]

The suggestion I've made there is to instead use the length of the stringified selection set as a hash function (as it's unaffected by order changes)

I think "stringifying" selection set is going to be rather slow for computing the keys. Why not just go with something simple as selection.key() + selection.selectionSet.selections().length instead? We are going to be comparing the selection sets on hits anyway, so first usingselection.key() we will limit to the same type condition + directives and then we would just need to quickly compare the size of selection set size to achieve the same thing.

It's a tradeoff between hash function speed and collision rate. You can indeed use the top-level field info of the selection set for the hash function instead of the entire selection set (it's like using the prefix of a string for a string's hash function instead of the entire string). The cost is that you may encounter more collisions, which means more spurious SelectionSet.equals() checks (asymptotically it means the algorithm leans more quadratically than linearly, though practically it's probably fine).

In any case this is a short-term fix under a feature flag, so I'm fine with whatever hash function we choose here as long as the collision rate isn't too high.

[sachindshinde]

Since the comparison is against the pre-minified selection set, I think you'll want the pre-minified selection set in the pair instead of the minified selection set. (The NamedFragmentDefinition should still use the minified selection set though.)

[trevor-scheer]

e8560f8

[dariuszkuc]

It's a tradeoff between hash function speed and collision rate. You can indeed use the top-level field info of the selection set for the hash function instead of the entire selection set (it's like using the prefix of a string for a string's hash function instead of the entire string). The cost is that you may encounter more collisions, which means more spurious SelectionSet.equals() checks (asymptotically it means the algorithm leans more quadratically than linearly, though practically it's probably fine).

Indeed there are way more complex hashing algos that we should look into in the future. Using the prefix just narrows down the selection sets to the same type condition and then we just check for number of selections there. I'd imagine instead doing the concatenating selection set into a string to get the final length we could simply recursively iterate over the selection sets and just aggregate the final length value (but unsure whether in practice it would be any better than recursively going through selection sets to calculate total number of selected fields).

[sachindshinde]

One minor nit to fix in a followup

[sachindshinde]

FragmentSelection is an abstract class implemented by InlineFragmentSelection and FragmentSpreadSelection, so I don't think you'll need to change the definition here.

[dariuszkuc]

#2960

[duckki]

I noticed that reuseQueryFragments option does not reuse fragments of size 1 (having only one field selection). It's probably because we don't get any compression by reusing those. Would the same logic apply with generateQueryFragments? I see tests are generating size-1 fragments. (Sorry I joined the party too late)

[dariuszkuc]

Indeed when reusing fragments we apply a logic to ensure we have selection set > 1. For fragment generation logic we decided to keep it simple for now (we could definitely improve it in the future and provide some better optimizations, e.g. join all sibling inline fragments into one named fragment etc).