Lazy look_ahead #1212

audunhalland · 2023-11-12T02:59:26Z

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:

trait LookAheadMethods removed. In my opinion there's no need for this and it always requires an explicit extra import.
struct ConcreteLookAheadSelection removed
struct LookAheadArguments removed, replaced by an opaque impl Iterator
struct LookAheadSelection now has some inherent methods at least partially compatible with LookAheadMethods:
- field_name, has_arguments: Compatible signature
- arguments now returns impl Iterator<Item = LookAheadArgument> instead of &[LookAheadArgument].
- argument now returns Option<LookAheadArgument> instead of Option<&LookAheadArgument>.
- children now returns LookAheadChildren instead of Vec<&Self>. LookAheadChildren implements IntoIterator.
- field_original_name / field_alias: New in 0.16
Some methods moved into new struct LookAheadChildren:
- LookAheadMethods::child_names
- LookAheadMethods::has_children moved to LookAheadChildren::is_empty
- LookAheadMethods::select_child moved to LookAheadChildren::select
LookAheadSelection::for_explicit_type moved to LookAheadSelection::children_for_explicit_type, which returns LookAheadChildren.
LookAheadValue::List(_) contains a new struct LookAheadList with IntoIterator + iter method.
LookAheadValue::Object(_) contains a new struct LookAheadObject with IntoIterator + iter method.

juniper/src/executor/look_ahead_lazy.rs

juniper/src/executor/mod.rs

juniper/src/executor/look_ahead_lazy.rs

audunhalland · 2023-11-18T22:15:04Z

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 · 2023-11-21T15:16:08Z

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

audunhalland · 2023-11-21T15:25:30Z

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

tyranron

@audunhalland the overall redesign looks interesting, and I'm much in favor to lazify things. Moreover, I'd like having it for the upcoming 0.16 release, so its benefirs could be described in the Book.

However, some API decisions should be thought, discussed and bikeshedded a little bit more, as I don't have strong opinion about them yet.

juniper/src/executor/look_ahead_lazy.rs

tyranron · 2024-01-15T17:04:48Z

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

* No need to wrap a &[T] in an Option only because a Default impl is needed * Make more things module-private * `S: 'a` bound not needed

…e intuitive

…ired

* make fn arguments() return impl Iterator<Item = LookAheadArgument> * re-introduce has_arguments() * remove struct LookAheadArguments (which was just an Iterator)

Iterators live in `pub mod iter`, an attempt to not clutter the look_ahead documentation page too much.

audunhalland · 2024-01-15T20:30:07Z

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?

tyranron

@audunhalland thank you for the hard work on this!

audunhalland force-pushed the lazy-look-ahead branch 2 times, most recently from 44813c8 to faaa987 Compare November 12, 2023 03:39