graphql-rust / juniper

GraphQL server library for Rust
Other
5.72k stars 425 forks source link

Lazy look_ahead #1212

Closed audunhalland closed 10 months ago

audunhalland commented 1 year ago

I've noticed that when using the look-ahead feature, this always involves a full traverse/transform of the input document under that node. This can be a problem when the input documents are large and complex.

This is an attempt to rewrite the look_ahead API so that its invocation becomes basically free. Instead of doing a forced deep transformation up front, the required work has been moved into the methods explicitly called by the API user.

A single level of LookAheadChildren gets computed when the user calls LookAheadSelection::children(). The arguments/values to a field are also lazily computed, and variable substitution is now done purely on demand.

The PR is in a draft state and the old look_ahead API is still intact for the time being, I'm interested in general feedback first before finishing it off.

Summary of API breakages:

audunhalland commented 1 year ago

I know the change looks large, I made it so because I essentially see it as a rewrite. I would like to get some indication on whether you would accept a change along these lines. If you are positive, I'll finish up the PR, overwrite the existing implementation and update the integration tests. I've finished this locally.

tyranron commented 1 year ago

@audunhalland I'll take a look in a week or so.

audunhalland commented 1 year ago

@tyranron Thanks! If you instead prefer a PR closer to the finished product, I can fix that before that time.

tyranron commented 10 months ago

@audunhalland once you finish adjustments, please, re-request my review via this GitHub button. Thanks!

Screenshot 2024-01-15 at 18 03 06
audunhalland commented 10 months ago

Ok, rebased on master, overwrote look_ahead and updated the tests I found to be relevant (based on grepping for look_ahead).

edit: I think the changelog should also be updated, maybe just use the change summary in the PR description?