drift icon indicating copy to clipboard operation
drift copied to clipboard

feat: Prefetch

Open dickermoshe opened this issue 1 year ago • 3 comments

TBD

dickermoshe avatar Jul 01 '24 04:07 dickermoshe

The code works as is, but it's not clean at all. I'm gonna take one or two days off this so that I can take a fresh look at it.

You could take a look at it now If you wish.

dickermoshe avatar Jul 01 '24 04:07 dickermoshe

@simolus3 Still needs docs and tests, but otherwise is complete. When I have time I'll write up a much clearer explanation of what going on. Sometime tomorrow

dickermoshe avatar Jul 01 '24 18:07 dickermoshe

🚧WIP🚧

This is how prefetching will work under the hood

Step 1 - The Callback

The user decides what he would like prefetch by using an optional call back in the withReferences method.

withReferences((o)=> o(prefetchUsers: true)).get()

Running the o function, will return a new function. What does this function do? What does it return?

Well, the return value from o is itself an async function.

o(prefetchUsers: true) // Future<Prefetches> Function()
await o(prefetchUsers: true)() // Prefetches

Calling it will return a Prefetches object.

Step 2 - Prefetches

Well, what is a prefetches? Relax, I'm getting to it. Each table has a prefetches object associated with it. (yes, even more code 😟) This holds the prefetched data. So running await o(prefetchUsers: true)() will return a class GroupsPrefetches which holds all the users for ALL the group.

Step 3 - OK, When are we running this?

Well, we don't run await o(prefetchUsers: true)() just yet. When we run withReferences((o)=> o(prefetchUsers: true)), we don't get the prefetches yet.

  1. The race conditions could be even worse
  2. withReferences would need to be async.

So when do we run them? We run them when we actually do the get()

So calling withReferences(...) saves the entire callback ((o)=>o(prefetchUsers: true)) for later. This is stored on another field of the model.

Step 4 - OK, I want my references now

Sure, when you finally run get(), we will run await o(prefetchUsers: true)() we will get the prefetches for this class. We will then insert this prefetches object into every <TableName>WithReferences object.

So when you finally run

final (group,refs)  = await groups.withReferences((o)=> o(prefetchUsers: true)).getSingle()
final users = await refs.users.get();

The refs object will insert the appropriate users from the prefetches into the cache of the newly created table manager (remember users is a table manager). Running .get() or .watch will return from this cache on refs.users.get(), instead of making a query.

If you don't understand this, it's ok. It will make sense once you see the code (I hope)

dickermoshe avatar Jul 01 '24 19:07 dickermoshe

Running the o function, will return a new function.

I see why this is helpful to store the callback, but it looks like additional complexity in the API. Couldn't we move that method into the generated manager classes instead of adding another type parameter? Then it would just be withReferences(users: true). Seems easier to me.

simolus3 avatar Jul 04 '24 18:07 simolus3

Any functionality that we want to work on child managers needs to be put into the TableManagerState. Otherwise

db.manager.users.withReferences(...) // Works
db.manager.users.filter(...).withReferences(...) // Does not work

What does filter() return? It could return the built in ProccessedManager , but that has no idea about our withReferences method. It could return a custom ProccessedUsersManager which extends the built in ProccessedManager, but that would require use to:

We would have RootUsersManager extend a BaseUsersManager which has a withReferences function on it. We would then need to have a ProccessedUsersManager extend this BaseUsersManager too. We would then need to include a callback on the table state for how to create a ProccessedUsersManager along with a generic for this ProccessedUsersManager.

It sorta used to work this way. But it was WAY WAY more confusing than it is now.

So we just return a built in ProccessedManager and put all needed callbacks in Generics and Callbacks on the table state

I agree the API is kinda ugly though. There is a tradeoff between how much code we generate and how user friendly it is. I might have overshot it.

Either way, when macros come out and generated code is not a concern, we can adjust many of these apis

dickermoshe avatar Jul 04 '24 19:07 dickermoshe

@simolus3 On second thought is does really look strange. I'm working on adding support for joins now. After that, I'll do what I've described above.

dickermoshe avatar Jul 04 '24 23:07 dickermoshe

After more thought, the idea of a tables with a cache presents too many foot guns to users.

  1. If you apply filters to manager with a cache, the cache is invalidated. This will create an N+1 issue.
  2. Streams won't be triggered on referenced tables.

I'm going to think about this

dickermoshe avatar Jul 07 '24 02:07 dickermoshe

@simolus3

In an effort to reduce code duplication, code generation length and readability, I spent some time seeing if using TypedResult could be a good place to put our prefetched data, and it turns out that it works great.

The only issue is that TypedResult doesn't have any public modifyers on it. I need some fields exposed to use it in generated code.

This would mark the 1st time I'm touching a file in your domain.

Does this look fine to you?

/// Returns a copy of this [TypedResult] instance with data appended to the [_parsedData] field
  ///
  /// This method is used by generated code and should not be used directly
  // ignore: non_constant_identifier_names
  TypedResult $_withData(ResultSetImplementation table, dynamic data) {
    return TypedResult(
      {..._parsedData, table: data},
      rawData,
      _parsedExpressions,
    );
  }

dickermoshe avatar Jul 08 '24 23:07 dickermoshe

If that's the cleanest way to go about this I'm fine with it, but some things look concerning, e.g.

  • Usually _parsedExpressions is expected to contain an entry for each column in the table, so that users can do read(myTable.myColumn) if they select from myTable. Since the manager doesn't publicly expose the TypedResult instance (or so I know), breaking that invariant may be fine. But it's something to be aware of.
  • I'm not sure if we should copy or just make this mutable if it's only used internally?

Is the problem that the original join builder doesn't include tables added with a "n+1" query?

simolus3 avatar Jul 09 '24 21:07 simolus3

Closed in favor of https://github.com/simolus3/drift/pull/3086

I think this is the 3rd draft I've thrown out 😂, hopefully it's the last.

dickermoshe avatar Jul 10 '24 13:07 dickermoshe