Skip to content

Add F[_] type parameter to DataSource - #171

Merged
purrgrammer merged 3 commits into
masterfrom
ds-identity
Feb 5, 2019
Merged

Add F[_] type parameter to DataSource#171
purrgrammer merged 3 commits into
masterfrom
ds-identity

Conversation

@purrgrammer

@purrgrammer purrgrammer commented Nov 21, 2018

Copy link
Copy Markdown
Contributor

In the old version of Fetch, one needed to define the data source as an
implicit object. This way, we ensured that each DataSource had only one instance using reference equality.

A data source used to look like the following:

implicit object Users extends DataSource[UserId, User]{
  // ...
}

Now that we've introduced an F[_] type parameter in DataSource we no
longer can define it as an object. Thus, we need a way to identify
requests to a data source to be able to perform optimizations.

Now there's a new trait called Data, that identifies requests for a
particular piece of data, and is used to identify and group requests for
the same data.

A Data definition looks like the following:

object Users extends Data[Int, User] {
  def name = "Users"
}

Then, when defining the data source for users, we have to specify which
Data the DataSource is fetching:

def usersSource[F[_] : ConcurrentEffect]: DataSource[F, Int, User] = new DataSource[F, Int,
User] {
  def data = Users

  def CF = ConcurrentEffect[F]

  // ...
}

Now that we have Data and DataSource we can create a Fetch por a User:

def fetchUser[F[_] : ConcurrentEffect](id: Int): Fetch[F, User] =
  Fetch(id, usersSource)

See the examples directory and the tests for plenty of examples.

Comment thread shared/src/main/scala/datasource.scala Outdated
q.query[Author].list
}
def transactor[F[_]: ConcurrentEffect]: F[Transactor[F]] =
for {

@diesalbla diesalbla Nov 27, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for {
createTransactor.flatTap(_.trans(dropTable *> createTable *> authors.traverse(addAuthor) )

The flatTap allows you to tee and use the output for a side-effectful computation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it but I got a type mismatch between doobie.h2.H2Transactor and doobie.Transactor.

Comment thread shared/src/main/scala/datasource.scala Outdated
* identity wasn't found, it won't appear in the keys.
*/
def batch[F[_] : ConcurrentEffect](ids: NonEmptyList[I]): F[Map[I, A]] =
def batch(ids: NonEmptyList[I])(implicit C: ConcurrentEffect[F]): F[Map[I, A]] =

@diesalbla diesalbla Nov 27, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, one problem with using implicit parameters is that you can not override them or remove them from a method in the subclass: the subclasses must keep a similar signature to the parent.

We could leave here no implementation, or add one that only used an Applicative constraint (to use with List.traverse), and then move the implementation using ConcurrentEffect to a different mix-in trait.

trait ConcurrentBatch[F[_]] extends DataSource[F] {
   implicit protected ConcF: ConcurrentEffect[F] 
   def batch(ids: NonEmptyList[I]): F[Map[I, A]] = /****/
}

This would allow you to decouple the tests from the ConcurrentEffect type class.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ConcurrentEffect type class is something we can't escape from since it'll be required by Fetch when running or creating fetches. Having the implicit ConcurrentEffect in scope is something that you want when defining data fetches, since very few of those will require only Applicative.

Remember that Fetch is a library for reordering and optimizing IO, in particular reading data. In practice, you will be using the ConcurrentEffect API to express resource allocation, asynchronous effects and lifting IO computations to F so I think it's fine.

@codecov

codecov Bot commented Jan 31, 2019

Copy link
Copy Markdown

Codecov Report

Merging #171 into master will increase coverage by 1.34%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
+ Coverage    95.2%   96.55%   +1.34%     
==========================================
  Files           6        6              
  Lines         167      174       +7     
  Branches        7        4       -3     
==========================================
+ Hits          159      168       +9     
+ Misses          8        6       -2
Impacted Files Coverage Δ
shared/src/main/scala/datasource.scala 100% <100%> (ø) ⬆️
shared/src/main/scala/fetch.scala 96.79% <100%> (+1.4%) ⬆️
shared/src/main/scala/cache.scala 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06556e7...59a45b8. Read the comment docs.

@codecov

codecov Bot commented Jan 31, 2019

Copy link
Copy Markdown

Codecov Report

Merging #171 into master will increase coverage by 1.36%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
+ Coverage    95.2%   96.57%   +1.36%     
==========================================
  Files           6        6              
  Lines         167      175       +8     
  Branches        7        6       -1     
==========================================
+ Hits          159      169      +10     
+ Misses          8        6       -2
Impacted Files Coverage Δ
shared/src/main/scala/datasource.scala 100% <100%> (ø) ⬆️
shared/src/main/scala/fetch.scala 96.79% <100%> (+1.4%) ⬆️
shared/src/main/scala/cache.scala 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06556e7...61a28be. Read the comment docs.

In the old version of Fetch, one needed to define the data source as an
implicit object. This way, we ensured that each DataSource had only one instance using reference equality.

A data source used to look like the following:

```scala
implicit object Users extends DataSource[UserId, User]{
  // ...
}
```

Now that we've introduced an `F[_]` type parameter in `DataSource` we no
longer can define it as an `object`. Thus, we need a way to identify
requests to a data source to be able to perform optimizations.

Now there's a new trait called `Data`, that identifies requests for a
particular piece of data, and is used to identify and group requests for
the same data.

A `Data` definition looks like the following:

```scala
object Users extends Data[Int, User] {
  def name = "Users"
}
```

Then, when defining the data source for users, we have to specify which
`Data` the `DataSource` is fetching:

```scala
def usersSource[F[_]]: DataSource[F, Int, User] = new DataSource[F, Int,
User] {
  def data = Users

  // ...
}
```

Now that we have `Data` and `DataSource` we can create a `Fetch` por a `User`:

```scala
def fetchUser(id: Int): Fetch[F, User] =
  Fetch(id, usersSource)
```

See the `examples` directory and the tests for plenty of examples.
Comment thread shared/src/main/scala/fetch.scala Outdated
val combined = acc.get(dsId).fold(
(ds, blocked)
)({
case (ds, req) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ds variable here shadows the one from line 136. Should they always be the same?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, perhaps we should use back-quotes? As it is right now, the second ds is a completely different variable, that just happens to have the same name.

Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
@purrgrammer
purrgrammer merged commit 02c3738 into master Feb 5, 2019
@purrgrammer
purrgrammer deleted the ds-identity branch February 5, 2019 11:43
@JorgeCastilloPrz

JorgeCastilloPrz commented Feb 5, 2019

Copy link
Copy Markdown

@purrgrammer can you open an issue on Kollect to replicate this in case it makes sense?

bijancn pushed a commit to bijancn/fetch that referenced this pull request Sep 19, 2019
Add F[_] type parameter to DataSource
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants