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

Collections support #758

Merged
merged 16 commits into from
May 7, 2017
Merged

Collections support #758

merged 16 commits into from
May 7, 2017

Conversation

mosyp
Copy link
Collaborator

@mosyp mosyp commented Apr 24, 2017

Fixes #706, #730, #309 and probably something more
This PR's brings collections support for row elements (SQL Arrays, Cassandra Collections)

SQL

  • Introduced support for SQL Arrays. They're mapped into quill as instances of Seq, which means we can represent them as any type which implements Seq.
  • Array support by default mixed into Postgres contexts

Cassandra

  • Introduced support of Cassandra's List, Set and Map. They're mapped into quill as immutable collections.
  • Introduced CassandraType[T] - marker which signals to quill that type T is already supported by Cassandra codecs so not additional mapping is needed
  • Introduced CassandraMapper[I, O] - additional mapper to/from CassandraType[T]
  • Provide implicit mappers for introducing CassandraMapper from MappingEncoding and CassandraType

We can use CassandraType[T] for custom codecs and reuse later on for UDT for example

Checklist

  • Jdbc arrays
  • Async arrays
  • Finagle arrays - won't do
  • Cassandra collections
  • Refactor array encodings
  • Unit test all changes
  • Update README.md if applicable
  • Add [WIP] to the pull request title if it's work in progress
  • Squash commits that aren't meaningful changes
  • Run sbt scalariformFormat test:scalariformFormat to make sure that the source files are formatted

@getquill/maintainers

@mosyp mosyp changed the title [WIP] Quill collection support [WIP] Quill collections support Apr 24, 2017
@mosyp mosyp changed the title [WIP] Quill collections support [WIP] Collections support Apr 24, 2017
@fwbrasil
Copy link
Collaborator

@mentegy this is great! Let us know if there's something we can help with.

@fwbrasil
Copy link
Collaborator

@mentegy it seems that @mxl started working on collections for the cassandra module: #644

@mosyp
Copy link
Collaborator Author

mosyp commented Apr 25, 2017

@fwbrasil Yes I saw it, but it seems to be inactive now. Also, I have a little bit another design idea as far as you can see from my latest commit. Please look at it, I have created a separate module for cassandra macros. It is already support primitive types (using boxing methods from Predef).
Next step is to create mappers for other cassandra supported AnyRefs types and MappedEncoding. If you have some idea regarding them please advice

@fwbrasil
Copy link
Collaborator

@mentegy cool, I'll take a look in the evening today

@mxl
Copy link
Contributor

mxl commented Apr 25, 2017

I started that PR long ago and did not have time to make much progress so it's nice that @mentegy will implement Cassandra collections support.

* Base trait for encoding sql arrays via async driver.
* We say `array` only in scope of sql driver. In Quill we represent them as instances of Traversable
*/
trait ArrayAsyncEncoding extends ArrayEncoding {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wdyt about having separate traits for encoders and decoders? I'd be more similar to the rest of the encoding classes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Make sense, will do


import scala.reflect.macros.blackbox.{ Context => MacroContext }

trait CollectionsEncodingMacro {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you share some context on why you need these macros for cassandra? It's not clear to me

Copy link
Collaborator Author

@mosyp mosyp Apr 26, 2017

Choose a reason for hiding this comment

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

On the first glance it looked easier to implement because of the following:
For Cassandra encoders/decoders we need a some kind of mapper:

  • boxing/unboxing primitives (this needs to be done manually)
  • identity function for supported types
  • mapped encoding

For list and set we can implement something like with sql arrays - collection encoder for each base type. But for map?
One solution is to introduce some type class which will mark supported types and pass each into implicit context. Also another encoders for AnyVals (oh, i completely forgot about anyvals) and primitives. And another one for mapping encoders. This looked a boilerplate for me for the first time and that's why I went into macros.
Having such "CassandraType[T]" will allow as to create generic encoder using cassandra row.set[T]. Also we can create some mechanism to create custom CassandraType[T] in order to create cassandra codecs for it.
Wdyt? I can play around it

Copy link
Collaborator Author

@mosyp mosyp left a comment

Choose a reason for hiding this comment

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

MappedType[T, Cas] is designed to map any scala type of T to already supported type Cas via cassandra codecs.
type CassandraType[T] = MappedType[T, T] - signals that this type T is already supported so no transformations is needed. Both of these type is needed for collection support. Later on we can reuse this for UDT for example

}

/* implicit def mappedEncodingForMappedType[I, O, Cas] is not working!!!
"Mapped encoding for mapped type" in {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This issue blocks me from complete cassandra collection support

m2: MappedEncoding[O, I],
mappedType: MappedType[O, Cas]
): MappedType[I, Cas] = MappedType(m1.f.andThen(mappedType.encode), mappedType.decode.andThen(m2.f))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This implicits is not working for unknowns reasons for me. Maybe I'm missing something?

mentegy added 6 commits April 28, 2017 16:23
- add tests in sql base module regarding arrays encoding
- add decoderUnsafe to MirrorDecoder to reduce ClassTags (removed unnecessary classtags from cassandra collection decoders)
@mosyp
Copy link
Collaborator Author

mosyp commented Apr 29, 2017

@fwbrasil I think it's completed. Please review.
Also check whereas updated info regarding changes in readme is sufficient.
Thanks!

@mosyp
Copy link
Collaborator Author

mosyp commented Apr 29, 2017

[tut] compiling: /app/target/tut/README.md
[tut] *** Error reported at /app/target/tut/README.md:706
<console>:28: error: not found: type LocalDate
       case class Person(id: Int, phones: List[String], cards: Vector[Int], dates: Seq[LocalDate])

Seems like tut doesn't like my example with LocalDate in readme. Also I found that #751 has the same problem.
Is there something we can do with tut or should I fix my code example in readme?

@mosyp
Copy link
Collaborator Author

mosyp commented May 2, 2017

@fwbrasil tut still cannot compile, it complains that mirror sql context does not have arrays encoding. Should I introduce arrays encoding for sql mirror context (it means thta for all idioms mirror will encode/decode arrays) or just make tut not compile that line?

@fwbrasil
Copy link
Collaborator

fwbrasil commented May 2, 2017

@mentegy I think it'd be nice if the mirror source also supports arrays, but I don't think it's a blocker. We can create a ticket and work on it later.

@mosyp
Copy link
Collaborator Author

mosyp commented May 2, 2017

@fwbrasil I have removed scala prefix from code examples to make it compile and will create new issue to introduce arrays support to mirror context once this PR will be merged. Thanks

@mosyp mosyp changed the title [WIP] Collections support Collections support May 2, 2017
Copy link
Collaborator

@fwbrasil fwbrasil left a comment

Choose a reason for hiding this comment

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

It's great that we don't need macros for cassandra! Thank you for the contribution, it's an important feature.

@mosyp
Copy link
Collaborator Author

mosyp commented May 5, 2017

@fwbrasil You're welcome!
Also thanks for approving this. Introducing CassandraType[T] will allow me to implement udt support for quill soon

@fwbrasil
Copy link
Collaborator

fwbrasil commented May 5, 2017

@getquill/maintainers let me know if you have objections since this is a relatively large change, I'm planning to merge it tomorrow.

@mxl
Copy link
Contributor

mxl commented May 7, 2017

Looks awesome 👍

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.

4 participants