Ordinal-based coproducts #1091

Baccata · 2023-07-17T12:21:22Z

Changes the internal models of Service and UnionSchema to be based on coproduct's ordinals, in order to allow for indexed-based dispatch (which should be more performant than map-based dispatch)
In particular, split the endpoint method in the service interface, so that the input can be accessed on its own.
Changes the model of Alt to carry a projector partial function. PartialFunction[U, A] is preferred to U => Option[A] because it doesn't necessarily induce an instantiation. It's also trivial to go from PartialFunction[U, A] to U => Option[A] via the .lift method
Amends the rendering logic to render ordinal values where they're needed (all operations, all union members)
Adds Schema.either and Schema.tuple smart-constructors

Amend documentation where needed
~~Render unions as sealed abstract classes instead of sealed traits~~ not doing it because of how it adds a field to the bytecode representation of ADT member classes which uses more memory
Generalise Schema.tuple via Boilerplate to handle all tuples from arity 2 to 22

Change Service/Union schemas to be ordinal based. This has the following benefits: 1. Allows for indexed-based random access instead of label-centric mapping. 2. Aligns our model with what Scala 3 does around Sum Mirrors, which are centered around ordinals. 3. Forces a projection function to be held by alternatives, which makes sense (arguably speaking)

…coproducts

Baccata · 2023-07-17T12:22:34Z

modules/core/src/smithy4s/schema/Alt.scala

@@ -26,7 +27,8 @@ import kinds._
 final case class Alt[U, A](
    label: String,
    schema: Schema[A],
-    inject: A => U
+    inject: A => U,
+    project: PartialFunction[U, A]


New projection function, which removes the need to have projector in Dispatcher

inject + project form something similar to a prism, wondering if it'd be useful to define it that way...

edit: at the time I was originally writing this, we didn't have #1103 - that other PR adds the primitives, so we could actually use them in alts

I thought about it and I don't really want to because it'd impact things too much. Moreover we're making the generation of optics an opt-in.

And we can't make the Alt a bonafide Prism and expose it to users because of recursion concerns. Alts need to be hidden in the computation of the schema.

hmm I don't know about the recursion concerns, can you elaborate?

other than that I get it

hmm I don't know about the recursion concerns, can you elaborate?

Actually I was partially wrong. What I meant was that :

val schema: Schema[A] = Schema.recursive { val aField = schema.optional[A]("a", _.a) Schema.struct(aField)(A(_)) }

Rendering it as follows to make aField visible to the external world doesn't work because aField refers to the initialised schema.

val aField = schema.optional[A]("a", _.a) val schema: Schema[A] = Schema.recursive { Schema.struct(aField)(A(_)) }

However we could potentially render it as follows, which works :

val schema: Schema[A] = Schema.recursive { Schema.struct(aField)(A(_)) } val aField = schema.optional[A]("a", _.a)

I'll sync with Jeff to explore an alternative to his PR after this one is merged.

Baccata · 2023-07-17T12:23:58Z

modules/core/src/smithy4s/schema/Schema.scala


-  def union[U](alts: Vector[Alt[U, _]])(dispatch: U => Alt.WithValue[U, _]): Schema.UnionSchema[U] =
-    Schema.UnionSchema(placeholder, Hints.empty, alts, dispatch)
+  def either[A, B](left: Schema[A], right: Schema[B]) : Schema[Either[A, B]] = {


new smart constructors for Schema[Either[A, B]] and Schema[Tuple[A, B]]

Baccata · 2023-07-17T12:24:47Z

modules/dynamic/src/smithy4s/dynamic/internals/DynamicService.scala

    hints: Hints
 ) extends Service.Reflective[DynamicOp]
    with DynamicSchemaIndex.ServiceWrapper {

  type Alg[P[_, _, _, _, _]] = PolyFunction5.From[DynamicOp]#Algebra[P]
  override val service: Service[Alg] = this

-  private lazy val endpointMap: Map[ShapeId, Endpoint[_, _, _, _, _]] =
-    endpoints.map(ep => ep.id -> ep).toMap
+  private lazy val ordinalMap: Map[ShapeId, Int] =


this could be changed by finding a way for the DynamicEndpoint to carry the ordinal

is there any issue with making it do so?

although tbh I don't mind it either way, the map seems just as fine

is there any issue with making it do so?

It's not that simple, because the DynamicOps are not currently tied to a service.

Baccata · 2023-07-17T12:25:34Z

project/Boilerplate.scala

@@ -222,21 +222,6 @@ object Boilerplate {
      -   final def andThen[H[${`_.._`}]](other: PolyFunction$suffix[G, H]): PolyFunction$suffix[F, H] = new PolyFunction$suffix[F, H]{
      -      def apply[${`A..N`}](fa: F[${`A..N`}]): H[${`A..N`}] = other(self(fa))
      -   }
-      -
-      -   import Kind$arity._


removing this because the unsafe caching needs to be backed by maps or arrays depending on the situation

daddykotex

that def either change makes it so much cleaner

good job once more

daddykotex · 2023-07-17T14:06:12Z

modules/aws-http4s/test/src/smithy4s/aws/query/AwsQueryCodecSpec.scala

-        case Right(string) => right(string)
-      }
-    }
+    implicit val schema: Schema[Foo] = Schema.either(int, string)


that looks good, wow

Baccata · 2023-07-18T08:49:20Z

modules/core/src/generated/schema/PartiallyAppliedTuple.scala

+package smithy4s
+package schema
+
+class PartiallyAppliedTuple protected[schema](placeholder: ShapeId) {


Allows for Schema.tuple(schemaA, schemaB, ...)

Baccata · 2023-07-19T08:06:14Z

@kubukoz if you can give it a review, I'd appreciate it :)

kubukoz · 2023-07-19T16:13:02Z

I will

kubukoz · 2023-07-19T19:51:29Z

modules/core/src/smithy4s/schema/Alt.scala

@@ -26,7 +27,8 @@ import kinds._
 final case class Alt[U, A](
    label: String,
    schema: Schema[A],
-    inject: A => U
+    inject: A => U,
+    project: PartialFunction[U, A]


inject + project form something similar to a prism, wondering if it'd be useful to define it that way...

edit: at the time I was originally writing this, we didn't have #1103 - that other PR adds the primitives, so we could actually use them in alts

kubukoz · 2023-07-19T19:51:37Z

modules/core/src/smithy4s/schema/Alt.scala

@@ -59,10 +61,23 @@ final case class Alt[U, A](
 object Alt {

  final case class WithValue[U, A](


question: can we remove this now? I don't see anything failing to compile when I do this

diff --git a/modules/core/src/smithy4s/schema/Alt.scala b/modules/core/src/smithy4s/schema/Alt.scala index 45eb6db6..20e41ad0 100644 --- a/modules/core/src/smithy4s/schema/Alt.scala +++ b/modules/core/src/smithy4s/schema/Alt.scala @@ -34,8 +34,8 @@ final case class Alt[U, A]( @deprecated("use .schema instead", since = "0.18.0") def instance: Schema[A] = schema - def apply(value: A): Alt.WithValue[U, A] = - Alt.WithValue(this, value) + // def apply(value: A): Alt.WithValue[U, A] = + // Alt.WithValue(this, value) def hints: Hints = schema.hints def memberHints: Hints = schema.hints.memberHints @@ -60,10 +60,10 @@ final case class Alt[U, A]( } object Alt { - final case class WithValue[U, A]( - private[schema] val alt: Alt[U, A], - value: A - ) + // final case class WithValue[U, A]( + // private[schema] val alt: Alt[U, A], + // value: A + // ) type ClassTagged[U] = WithClassTag[U, _] final case class WithClassTag[U, A <: U](

kubukoz · 2023-07-19T19:52:25Z

modules/core/src/smithy4s/schema/Alt.scala

      value: A
  )

+  type ClassTagged[U] = WithClassTag[U, _]


nitpick: I don't see this type being used anywhere

kubukoz · 2023-07-19T19:56:04Z

modules/core/src/smithy4s/schema/Alt.scala

+  final case class WithClassTag[U, A <: U](
+      alt: Alt[U, A],
+      classTag: scala.reflect.ClassTag[A]
+  )
+
+  object WithClassTag {
+    implicit def fromAlt[U, A <: U: ClassTag](
+        alt: Alt[U, A]
+    ): WithClassTag[U, A] =
+      WithClassTag(alt, implicitly[ClassTag[A]])
+  }


question: are these used anywhere?

kubukoz · 2023-07-19T19:57:53Z

modules/core/src/smithy4s/schema/PartiallyAppliedUnion.scala

+
+import smithy4s.Hints
+
+class PartiallyAppliedUnion[U](val alts: Vector[Alt[U, _]]) extends AnyVal {


nitpick: final class?

kubukoz · 2023-07-20T15:32:27Z

modules/core/src/smithy4s/http/HttpErrorSelector.scala

+      def apply[A](alt: Alt[E, A]): F[E] = {
+        resultCache(alt).asInstanceOf[F[E]]


question: is this something we could encapsulate in a public pattern? I suppose it'd be a sort of dual to Dispatcher

We might be able to but I've not thought about it enough to know if it's possible

Right, the bare minimum for us to be able to generalise would be to have a flatmap constraint on the F

I'd be fine with such a constraint. flatten + map (from Covariant) would be fine, no?

I gave it a go and it's a lot more complicated to generalise than the encoding side, partially because the action of decoding may "consume" data.

So it ends up looking like this :

def compileDecoder[Decoder[_], F[_], Message, Discriminator]( precompile: Precompiler[Decoder], discriminator: Alt[_, _] => Discriminator, discriminate: Message => F[(Discriminator, Message)] )(implicit decoder: DecoderK[Decoder, F, Message] ): Decoder[U]

It may be worth exploring further but certainly not in this PR

kubukoz · 2023-07-20T15:35:58Z

modules/core/src/smithy4s/schema/Alt.scala

@@ -86,51 +101,48 @@ object Alt {
        encoderK: EncoderK[G, Result]
    ): G[U]

-    def projector[A](alt: Alt[U, A]): U => Option[A]
+    def ordinal(u: U): Int


thought: could we make it more constrained than just Int, in dispatchers? maybe a type member?

then, we'd also need methods like def get[A](i: Ordinal)(items: Seq[A]): A.

We could have get[A](u: U)(items: Seq[A]): A ?

that... works. but now I'm afraid of people passing a seq of the wrong length. It's equally bad :(

the whole idea is reminiscent of Representable from cats

kubukoz · 2023-07-20T15:38:29Z

modules/dynamic/src/smithy4s/dynamic/internals/DynamicService.scala

    hints: Hints
 ) extends Service.Reflective[DynamicOp]
    with DynamicSchemaIndex.ServiceWrapper {

  type Alg[P[_, _, _, _, _]] = PolyFunction5.From[DynamicOp]#Algebra[P]
  override val service: Service[Alg] = this

-  private lazy val endpointMap: Map[ShapeId, Endpoint[_, _, _, _, _]] =
-    endpoints.map(ep => ep.id -> ep).toMap
+  private lazy val ordinalMap: Map[ShapeId, Int] =


is there any issue with making it do so?

kubukoz · 2023-07-20T15:38:43Z

modules/dynamic/src/smithy4s/dynamic/internals/DynamicService.scala

    hints: Hints
 ) extends Service.Reflective[DynamicOp]
    with DynamicSchemaIndex.ServiceWrapper {

  type Alg[P[_, _, _, _, _]] = PolyFunction5.From[DynamicOp]#Algebra[P]
  override val service: Service[Alg] = this

-  private lazy val endpointMap: Map[ShapeId, Endpoint[_, _, _, _, _]] =
-    endpoints.map(ep => ep.id -> ep).toMap
+  private lazy val ordinalMap: Map[ShapeId, Int] =


although tbh I don't mind it either way, the map seems just as fine

kubukoz · 2023-07-20T15:40:01Z

modules/core/src/generated/schema/PartiallyAppliedTuple.scala

+package smithy4s
+package schema
+
+class PartiallyAppliedTuple protected[schema](placeholder: ShapeId) {


…coproducts

Baccata added 5 commits July 17, 2023 11:59

Merge remote-tracking branch 'origin/series/0.18' into ordinal-based-…

006c962

…coproducts

Re-introduce a simplified endpoint method

6746c40

cleanup

49b1abe

headers

30c5156

Baccata commented Jul 17, 2023

View reviewed changes

Baccata added 4 commits July 17, 2023 15:06

Add a reflective method for creating unions

ca9d9dc

Fix some tests

8521042

Fix dynamic tests

c56c408

Fix decline tests

03ae4d1

daddykotex reviewed Jul 17, 2023

View reviewed changes

Baccata added 6 commits July 17, 2023 21:21

Fix cats tests

7f47215

Fix dynamic stability

687e8b5

Fix xml tests

68e81b6

Amend reflective method to explicitly throw a match error

128d4e2

Amend documentation to mention ordinal

1beb752

Add Schema tuples for all arities

dde9956

Baccata commented Jul 18, 2023

View reviewed changes

Baccata marked this pull request as ready for review July 19, 2023 08:05

kubukoz self-requested a review July 19, 2023 16:13

kubukoz reviewed Jul 20, 2023

View reviewed changes

Baccata added 4 commits July 24, 2023 14:19

Merge remote-tracking branch 'origin/series/0.18' into ordinal-based-…

bd9c5c9

…coproducts

cleanup

79428df

relax Vector => IndexedSeq in Service interface

8a316cc

Merge remote-tracking branch 'origin/series/0.18' into ordinal-based-…

bac6b5d

…coproducts

Baccata added 2 commits July 24, 2023 16:04

Fix String reference

344f4ff

regenerate bootstrapped sources

3f0315d

kubukoz approved these changes Jul 24, 2023

View reviewed changes

Baccata merged commit 8b8cba7 into series/0.18 Jul 24, 2023
10 checks passed

Baccata deleted the ordinal-based-coproducts branch July 24, 2023 15:47

		@@ -59,10 +61,23 @@ final case class Alt[U, A](
		object Alt {

		final case class WithValue[U, A](


		import smithy4s.Hints

		class PartiallyAppliedUnion[U](val alts: Vector[Alt[U, _]]) extends AnyVal {

		def apply[A](alt: Alt[E, A]): F[E] = {
		resultCache(alt).asInstanceOf[F[E]]

Ordinal-based coproducts #1091

Ordinal-based coproducts #1091

Conversation

Baccata commented Jul 17, 2023 • 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

Baccata Jul 24, 2023 • 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

Baccata Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

daddykotex left a 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

Baccata commented Jul 19, 2023

kubukoz commented Jul 19, 2023

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

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

Baccata commented Jul 17, 2023 •

edited

Loading

Baccata Jul 24, 2023 •

edited

Loading

Baccata Jul 17, 2023 •

edited

Loading