Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add array support for FinaglePostgresContext #1123

Open
davoclavo opened this issue Jun 29, 2018 · 5 comments
Open

Add array support for FinaglePostgresContext #1123

davoclavo opened this issue Jun 29, 2018 · 5 comments

Comments

@davoclavo
Copy link

davoclavo commented Jun 29, 2018

Version: (e.g. 2.5.4)
Module: (e.g. quill-finagle-postgres)
Database: (e.g. postgres)

Expected behavior

It would be nice if FinaglePostgresContext had Array support, such as PostgresJdbcContext and PostgresAsyncContext

Actual behavior

Steps to reproduce the behavior

case class User(nicknames: Seq[String])
create table user(
  nicknames text[]
)
ctx.run(query[User])

Workaround

I guess write your own Encoder/Decoder. Not sure if it is possible to write such decoders for container types like Seq[T]

@getquill/maintainers


I will try to implement similar traits like ArrayEncoders extends ArrayEncoding and ArrayDecoders extends ArrayDecoding from the async/jdbc postgres contexts.

@davoclavo
Copy link
Author

davoclavo commented Jun 29, 2018

Quick update, I was able to get a very early stab at building an array encoder (Still missing the binary representation, and the decoder part) -- It seems that it is trickier to extend ArrayEncoding than to just implement the encoder similar to the one for Option[T] and also getting inspiration from finagle-postgres' Option[T] ValueEncoder

trait ArrayEncoders  {
  self: FinaglePostgresContext[_] =>

  implicit def array[T](implicit encodeT: ValueEncoder[T]): ValueEncoder[Seq[T]] =
    new ValueEncoder[Seq[T]] {
      val typeName = "array"
      val elemTypeName = None

      def encodeText(seqT: Seq[T]) =
        Some(seqT.flatMap(encodeT.encodeText).mkString("{", ",", "}"))

      def encodeBinary(tSeq: Seq[T], c: Charset) = ???
    }

  implicit def arrayEncoder[T](implicit e: Encoder[T]): Encoder[Seq[T]] =
    FinanglePostgresEncoder[Seq[T]](array(e.encoder))
}

Note: I'm not yet sure if I should be encoding an empty array as null or {}


I am able to do things like:

case class User(nicknames: Seq[String])
val user = User(Seq("davoclavo", "david"))
ctx.run(quote{
  query[User].insert(lift(user))
})

Which inserts the data as {davoclavo,david}

@pettyjamesm
Copy link
Contributor

Definitely don't encode empty arrays as null, you lose the ability to distinguish between the two. Would you encode an empty string as null? I don't know anything about the finagle postgres implementation so sadly I can't help with that, but if it supports the same textual representation as arrays then that should still be a nice improvement for other users.

@davoclavo
Copy link
Author

davoclavo commented Jun 30, 2018

@pettyjamesm agree, thanks for the feedback! as soon as I did that I realized that it was a bad idea. This was a very quick draft on how I was attempting the implementation, mostly for future reference (I've updated the snippet to remove those null encodings)

@pettyjamesm
Copy link
Contributor

Don't worry, no judgement on rough drafts. Just wanted to give feedback on the part that I could and I'm sorry I couldn't be more useful regarding the meat of the problem.

@mosyp
Copy link
Collaborator

mosyp commented Jul 8, 2018

Hello @davoclavo, this one is better to be done in their driver repo, I've opened an issue while ago, see finagle/finagle-postgres#55.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants