[graphql-js] Yielding batches of results in resolver functions

[robrichard]

Moved from (#38)

Context

graphql-js will be updated to allow returning an AsyncIterable from list field resolvers. This is a prerequisite to supporting the @stream directive. What type should this AsyncIterable yield?

Option A: Single value: AsyncIterable<T>

Option B: Lists of values: AsyncIterable<Iterable<T>>

This was discussed in #38 and at the May 2022 WG meeting. From the working group meeting it was determined that it is desirable to for these resolver functions to yield batches of values.

Decision

Discussed at the May 2022 GraphQL-JS working group and there was general agreement that we should allow both individual results and batches of results. The default will be individual results and the mechanism used to support batching is still to be determined (see comment #40 (comment)). The group agreed that we can safely merge Option A to graphql-js to unblock further progress on defer/stream, and batching support can be championed separately.

References

• graphql-js pull request
• Outside GraphQL: https://medium.com/netscape/async-iterators-these-promises-are-killing-my-performance-4767df03d85b

[robrichard]

(Moved from #38)

I think there will be many cases where underlying api yields single values and many other cases where the underlying api yields lists of values. One example of a database api that yields lists of values is the DynamoDB paginateQuery API.

Option A: If we choose to yield single values, servers will need to convert apis the yield lists to yield each individual value.

Option B: If we choose to yield lists, servers will need to convert apis that yield individual values to wrap each value in a list before yielding.

With Option A, we remove the ability for the GraphQL execution to synchronously process more than one value at a time. While the resolver has access to multiple values, each value will have the overhead of an event loop tick added before the GraphQL execution can start processing it.

Option B doesn't introduce any overhead, but requires more code for what might be the simpler or more common use-case.

I think the tradeoffs for Options B are worth it. Some number of users will have to adapt their resolver code to fit whatever pattern we decide on, so it makes sense to me to favor the one that is more efficient and flexible.

[yaacovCR]

@robrichard fact is. With stream, only 1 item can emit in an instant. So even though your resolver is yielding lists, graphql will be flushing singles (the transport may contain more than one chunk, but still single line entries).

See #41 . It was also decided at the last WG meeting that graphql should flush all available payloads.

Why not keep it all the same as the spec?

So this will be the same as the spec.

And even with defer, you're still flushing 1 thing, that thing may be a list, or may also not be.

There could be multiple available deferred payloads; in fact, this could be quite common, if they all depend on similar dataloaders. It requires further experimentation to see how common it would be..

[benjie]

TL;DR: Option C: As option A, but with introduction of "Batch" type.

I am a proponent of allowing the return of lists to enable inherent batching - as you rightly point out, the underlying APIs are likely being batched (fetching 10 or 100 rows at a time from the source, for example) so allowing this to be expressed should keep this effiency throughout the stack rather than introducing inefficiency for the sake of simplicity.

However, with that being said, I have concerns. My primary concern is that in GraphQL.js the resolver for a field defined to return a list (e.g. allUsers: [User!]) can return an array of users, or an iterable each entry of which is a user. We're now suggesting that the field could also return an async iterable, each entry of which is a list of users. This feels unintuitive since a user would typically assume that going from an iterable to an async iterable would be the same shape, just (effectively) with promises. Assuming they have a generator function* values(){...}, putting the word async infront of it is not something they'd expect to break the result format. It feels like whatever we do, GraphQL.js should handle arrays, iterables and async iterables the same - the resolver itself should not need to know or care that @stream was applied, that is something handled for it by the GraphQL library.

One option is to automatically coerce between these formats; currently the GraphQL spec states in CompleteValue() that if the field type is a list type and the returned value is not a collection of values then a field error should be raised, which makes it relatively safe for us to do this. However there's immediate ambiguities for nested lists e.g. [[User!]!] - especially when the resolver returns data in an inconsistent format (which should raise a field error) - and it also would fall down if we softened this constraint in the spec in future (unlikely).

The Option B proposal is that the return type for the async iterable is concretely different to that of the regular iterable. It looks like that's safe to do currently (will not break existing servers), but is unintuitive, and it's a divergence from how async iterators work for subscriptions. Doing it has the pains outlined above, however using Option A pushes the problem of batching to a separate layer.

Despite it's problems, I still feel option B would be the correct course if we could only follow one of these two courses (system efficiency is more important to me than the issues of iterable/async iterable inconsistency)... but I wonder if there may be a third option somewhere? We could, for example, have a new type called Batch (or similar) and it could be returned as part of lists, iterables, and async iterables and handled the same in all cases. That way we'd have the Option A type, but with this new "Batch" type added everywhere. So:

// Pseudocode
type ListResolverResult<T> = Iterable<BatchOrDirect<T>> | AsyncIterable<BatchOrDirect<T>>
type BatchOrDirect<T> = T | Batch<T>

[yaacovCR]

Ivan has suggested previously that we introduce a new class with instanceof check to handle batches which seems similar to this suggestion. Great minds think alike. I prefer simply forcing clients to return arrays. In general, I am against additional instanceof checks, although perhaps duck/typing could be used instead.

Importantly, I think that this type of batching would be useful even when stream is not used, to avoid repetitively hitting the event loop. It might be even more useful in that case.

I do wonder if in retrospect this might be useful for subscriptions and the best upgrade path there...

[benjie]

Yes; using this with subscriptions is also something I would like to do 👍

I don't believe we can do safe duck-typing since resolvers can return basically anything at this point. We could, perhaps, make it so that you attach a given unique symbol to your return value (rather than using instanceof for example), but honestly I think using a GraphQL helper like makeBatch(iterable) would be better - leave it up to GraphQL.js to decide the internals of it (instanceof vs property check vs...)

[yaacovCR]

Actually, the current implementation of subscriptions precludes any benefit of this type of batching -- I believe -- as the generator graphql returns only yields single items and there is a guarantee that each item is completed prior to initiation of completion of the next. Is that something you are also interested in changing?

[yaacovCR]

I glanced at the spec and I don’t see that this behavior is required explicitly, although there seems to be an assumption that the result stream and event stream are in the same order… and I guess you can read the algorithm that the payloads have to be completed serially and not in parallel — although imo this could be more explicit.

But certainly a spec change would be required to implement batching for subscriptions…

Implementation wise, I think we could simply introduce a breaking change that yielding an array would be equivalent to yielding multiple values. As I don’t believe that there are very many root values that are arrays and I don’t think we run into the nested list issue there.

although this would introduce a difference between subscription and non subscription async generator resolvers that is non-desirable. We could teach it that with subscriptions, you might want execution of an event to occur in series, but you would never want that with queries

[robrichard]

@benjie @IvanGoncharov

TL;DR: Option C: As option A, but with introduction of "Batch" type.

Does this option unblock the current PR for adding AsyncIterable support (implementing Option A)? graphql/graphql-js#2757

This would allow us to move forward with this feature and defer/stream while separately working out the implementation details of makeBatch or however we decide to do it.

[benjie]

If we can get enough support for it, it may well do. We still need to handle it at the result/transport layer though.

[robrichard]

I added this to the next GraphQL-JS WG agenda: graphql/graphql-js-wg#85

[yaacovCR]

Option D:

Writers of resolver functions should know whether they are using batching or not. So the signature of the resolver should not be:

AsyncIterator<ItemType | Array<ItemType>> but rather AsyncIterator<ItemType> | AsyncIterator<Array<ItemType>>

I.e., rather than checking at runtime to see whether each entity yielded is an item or array of items, developers should somehow annotate the resolver to indicate whether batching is going to be used. One way this can be done is by using the standard resolve field when an AsyncIterator yielding single items is used, and a resolveWithBatching (or batchResolve? or resolveBatches?) field when the AsyncIterator returns Iterables of items.

Similarly, for subscriptions: subscribe and batchSubscribe? I am less confident whether for long-running subscriptions it will be known in advance whether the subscription solely using batching or not...