Rework Read/Write & Fix derivation to use custom instances #2136
Conversation
jatcwang
force-pushed
the
fix_auto_derivation
branch
2 times, most recently
from
2076053
to
1453bf1
Compare
| accum += rr.unsafeGet(rs, idx) | ||
| idx += rr.length | ||
| } | ||
| construct(accum.toList) |
There was a problem hiding this comment.
If accum is being turned into a list is it more efficient to use ListBuffer instead of ArrayBuffer?
There was a problem hiding this comment.
Nice - that seems to give a 20% boost in the benchmarks :)
|
|
||
| } | ||
|
|
||
| /** Simple instance wrapping a Put. i.e. single column nullable value */ |
| implicit def fromGet[A](implicit ev: Get[A]): Read[A] = | ||
| new Read(List((ev, NoNulls)), ev.unsafeGetNonNullable) {} | ||
| /** Simple instance wrapping a Put. i.e. single column non-null value */ | ||
| final case class Single[A](get: Get[A]) extends Read[A] { |
There was a problem hiding this comment.
Is there any need for this to be a case class instead of a class? It has no concept of equality and its not data.
There was a problem hiding this comment.
Thanks I've changed this for for both Read & Write
| @@ -121,7 +151,7 @@ class ReadSuite extends munit.FunSuite with ReadSuitePlatform { | |||
| // This result doesn't seem ideal, because we should know that Int isn't | |||
| // nullable, so the correct result is Some((1, None, 3, None)) | |||
| // But with how things are wired at the moment this isn't possible | |||
There was a problem hiding this comment.
Could remove this comment now
| scalacOptions ++= Seq( | ||
| "-Wconf:cat=unused-nowarn:s" | ||
| ), |
There was a problem hiding this comment.
The problem with the global "-Wconf:cat=unused-nowarn:s" setting is that it is applied everywhere, regardless of each particular case.
Another approach that could allow to tackle this on a case-by-case basis is to make use of scalac-compat – it has nowarn213 and nowarn3 which, if applied, would emit @nowarn for Scala 2.13 and Scala 3 respectively, and skip it for Scala 2.12.
AFAIR, scalac-compat is provided automatically by sbt-typelevel.
There was a problem hiding this comment.
Thanks I didn't know about the scalac-compat. I gave it a go but unfortunately somehow while compiling in Scala 3 it thinks @nowarn213 is unused :/
[warn] -- [E198] Unused Symbol Warning: /home/jacob/proj/contrib/doobie/modules/core/src/test/scala/doobie/util/ReadSuite.scala:14:45
[warn] 14 |import org.typelevel.scalaccompat.annotation.nowarn213
[warn] | ^^^^^^^^^
[warn] | unused import
Despite it being used in
(I changednowarn to nowarn213)
jatcwang
force-pushed
the
fix_auto_derivation
branch
from
1453bf1
to
25dfdd3
Compare
Fix both semiauto and automatic derivation to use custom defined Read/Write instances (e.g. in companion objects). A complete rework of Read and Write is unfortunately necessary because with the previous implementation, we cannot simply derive a `Read[Option[A]]` given a `Read[A]` - we'd need to derive `Option[A]` from scratch by resolving `Read[Option[X]]` instances for each of `A`'s columns. After the rework, both `Read` and `Write` are now sealed traits, each with 3 subclasses: - Single: Wrapper over a `Get/Put` - SingleOpt: Wrapper over a `Get/Put`, but is nullable i.e. `Read[Option[A]]`, `Write[Option[A]]` - Composite: A composite of `Read/Write` instances Apart from enabling proper semiauto and automatic derivation, the rework also brings these benefits: - Derivation rules are much simpler (which also translates to faster compile times). In particular, given a `Read/Write[A]` we can trivially derive `Read/Write[Option[A]]`. - We now correctly handle optionality for `Option[CaseClassWithOptionalAndNonOptionalFields]`. More tests will be added for this in a follow up PR to demonstrate Other notes: - Semiauto and Auto derivation of unary product type (e.g. 1 element case class) are removed due to it causing auto derivation to pick the wrong path. It seems unnecessary since Write/Read derivation will yield the same behaviour anyway? Fixes #1054, #2104
jatcwang
force-pushed
the
fix_auto_derivation
branch
from
25dfdd3
to
21e03ed
Compare
Fix both semiauto and automatic derivation to use custom defined Read/Write instances (e.g. in companion objects).
A complete rework of Read and Write is unfortunately necessary because with the previous implementation, we cannot simply obtain a
Read[Option[A]]given aRead[A]- we'd need to deriveOption[A]from scratch by resolvingRead[Option[X]]instances for each ofA's columns.After the rework, both
ReadandWriteare now sealed traits, each with 3 subclasses:Get/PutGet/Put, but is nullable i.e.Read[Option[A]],Write[Option[A]]Read/WriteinstancesApart from enabling proper semiauto and automatic derivation, the rework also brings these benefits:
Read/Write[A]we can trivially deriveRead/Write[Option[A]].Option[CaseClassWithOptionalAndNonOptionalFields]. More tests will be added for this in a follow up PR to demonstrateOther notes:
Fixes #1054, #2104