Rework Read/Write & Fix derivation to use custom instances #2136

jatcwang · 2024-11-13T20:14:58Z

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 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

guymers · 2024-11-18T10:34:43Z

modules/core/src/main/scala/doobie/util/read.scala

+        accum += rr.unsafeGet(rs, idx)
+        idx += rr.length
+      }
+      construct(accum.toList)


If accum is being turned into a list is it more efficient to use ListBuffer instead of ArrayBuffer?

Nice - that seems to give a 20% boost in the benchmarks :)

guymers · 2024-11-18T10:35:29Z

modules/core/src/main/scala/doobie/util/read.scala

+
+  }
+
+  /** Simple instance wrapping a Put. i.e. single column nullable value */


"Put" -> "Get"?

Fixed thanks

guymers · 2024-11-18T10:37:40Z

modules/core/src/main/scala/doobie/util/read.scala

-  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] {


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.

Thanks I've changed this for for both Read & Write

guymers · 2024-11-18T10:43:34Z

modules/core/src/test/scala/doobie/util/ReadSuite.scala

@@ -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


Could remove this comment now

satorg · 2024-11-18T19:27:28Z

build.sbt

+  scalacOptions ++= Seq(
+    "-Wconf:cat=unused-nowarn:s"
+  ),


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.

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

doobie/modules/core/src/test/scala/doobie/util/ReadSuite.scala

Line 122 in 25dfdd3

}: @nowarn("msg=.*(u|U)nused import.*")

(I changed nowarn to nowarn213)

jatcwang · 2024-11-29T20:56:46Z

modules/core/src/main/scala/doobie/util/read.scala

+  }
+
+  /** A Read instance consists of multiple underlying Read instances */
+  class Composite[A, S0, S1](read0: Read[S0], read1: Read[S1], f: (S0, S1) => A) extends Read[A] {


After some benchmarking, I have reworked Composite to be structured more like HLists rather than List[Read[Any]].
With List[Read[Any]] my understanding is that JVM cannot effectively perform inlining because due to the dynamic length/type nature of the list.
But with a HList-like structure (e.g. Composite(Read[Int], Composite(Read[String], Read[Int]))) the call graph and types are "constant" which I think made the JIT happier

Using benchmark from #2139, the current implementation has basically no penalty between Read[A] and Read[Option[A]], whereas the previous implementation has around a 50% performance penalty.

[info] Benchmark Mode Cnt Score Error Units [info] LargeRow.autoDerivedComplex thrpt 5 956584.142 ± 105909.280 ops/s [info] LargeRow.autoDerivedComplexOpt thrpt 5 925193.390 ± 240656.559 ops/s [info] LargeRow.rawJdbcComplex thrpt 5 1390634.422 ± 195647.819 ops/s [info] LargeRow.rawJdbcTuple thrpt 5 1341330.033 ± 248449.784 ops/s [info] LargeRow.semiautoDerivedComplex thrpt 5 957341.195 ± 155499.630 ops/s [info] LargeRow.semiautoDerivedComplexOpt thrpt 5 917308.756 ± 181313.503 ops/s [info] LargeRow.tuple thrpt 5 1068742.195 ± 208005.075 ops/s [info] LargeRow.tupleOpt thrpt 5 1072509.030 ± 177644.605 ops/s

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 2 times, most recently from 2076053 to 1453bf1 Compare November 13, 2024 20:46

guymers approved these changes Nov 18, 2024

View reviewed changes

satorg reviewed Nov 18, 2024

View reviewed changes

jatcwang force-pushed the fix_auto_derivation branch from 1453bf1 to 25dfdd3 Compare November 18, 2024 20:57

jatcwang mentioned this pull request Nov 21, 2024

Regression with Option[Record] results in 1.0.0-RC6 #2144

Open

jatcwang force-pushed the fix_auto_derivation branch 2 times, most recently from 21e03ed to 0360375 Compare November 26, 2024 21:38

jatcwang commented Nov 29, 2024

View reviewed changes

jatcwang force-pushed the fix_auto_derivation branch 2 times, most recently from 2b0b963 to 41cb935 Compare December 4, 2024 21:44

jatcwang force-pushed the fix_auto_derivation branch from 41cb935 to 8a83120 Compare December 7, 2024 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework Read/Write & Fix derivation to use custom instances #2136

Rework Read/Write & Fix derivation to use custom instances #2136

jatcwang commented Nov 13, 2024 •

edited

Loading

guymers Nov 18, 2024

jatcwang Nov 18, 2024

guymers Nov 18, 2024

jatcwang Nov 18, 2024

guymers Nov 18, 2024

jatcwang Nov 18, 2024

guymers Nov 18, 2024

jatcwang Nov 18, 2024

satorg Nov 18, 2024

jatcwang Nov 18, 2024 •

edited

Loading

jatcwang Nov 29, 2024


		}

		/** Simple instance wrapping a Put. i.e. single column nullable value */

Rework Read/Write & Fix derivation to use custom instances #2136

Are you sure you want to change the base?

Rework Read/Write & Fix derivation to use custom instances #2136

Conversation

jatcwang commented Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jatcwang Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jatcwang commented Nov 13, 2024 •

edited

Loading

jatcwang Nov 18, 2024 •

edited

Loading