Support Sets in FromMappable and ToMappable

[lacarvalho91]

This can be achieved the same way as done in #11 which would add support for maps too, however I have found that to be able to use a Set I also needed to define a CanBuild[T, Set[T]] so I guess the same would be true for Map

[nevillelyh]

I thought sets are already supported by #12?
https://github.com/nevillelyh/shapeless-datatype/pull/12/files#diff-2719185b103446764ee32fae7f1060e5R29

[lacarvalho91]

Yeah, for MapRecord but not FromMappable or ToMappable. This surfaced from using the avro module, when I changed a field type from List to Set I saw that I needed to define FromMappable and ToMappable instances for Iterable (as well as the CanBuild instance for Set)

[nevillelyh]

Ah you're right. Mind give it a try and submit a PR?

[lacarvalho91]

Yeah I plan on doing so 🙂have already defined them in my project and it seems to work. Will raise a PR later today.

[lacarvalho91]

@nevillelyh how are FromMappable and ToMappable tested? I'm having trouble working that out

[nevillelyh]

It's tested in concrete implementations like https://github.com/nevillelyh/shapeless-datatype/blob/master/avro/src/test/scala/shapeless/datatype/avro/AvroTypeSpec.scala

Map type only makes sense for Avro though since it supports that natively. For BigQuery TableRow and Datastore Entity it's just a nested row.

[lacarvalho91]

@nevillelyh OK cool, thanks!

[lacarvalho91]

@nevillelyh as soon as I change all the Seq instances to Iterable (mainly toSeq: S[V] => Seq[V] becomes toIterable: S[V] => Iterable[V]) then I get ambiguous implicit errors because for some reason that change makes it so that those instances match the expected type for Option. Which kind of makes sense since its just S[_], I'm confused as to how that change makes this surface though (since it is fine with Seq). If I then add a subtype constraint S[_] <: Iterable[_] it can't derive an instance for Array (this I don't understand either since in Predef there is an implicit conversion that turns an Array into a subtype of Seq) 😭 any suggestions?

[lacarvalho91]

Ah....that would be why

[nevillelyh]

Looked at the PR and this a bit. So basically you want Iterable to cover Set cases but that causes confusion with Option? I wonder if there's way to disable the Option => Iterable implicit here, otherwise we'll have to introduce our own type class and implement for all collection types except Option.

scala> implicitly[Option[Int] => Seq[Int]]
<console>:12: error: No implicit view available from Option[Int] => Seq[Int].
       implicitly[Option[Int] => Seq[Int]]
                 ^

scala> implicitly[Option[Int] => Iterable[Int]]
res8: Option[Int] => Iterable[Int] = $$Lambda$1160/1816269091@38e00b47

[nevillelyh]

@lacarvalho91 also still curious what's the use case for Sets. None of the use cases in this repo, e.g. Avro, BigQuery, Datastore, TF.Example supports Set data type. Do you intent to use it on something else?

[lacarvalho91]

@nevillelyh we're decoding Avro with an array field but its encoded as a Set in our case class as we don't want duplicates, we'd rather make duplicates impossible with types other than having to distinct or something and would rather we could just decode it as a set instead of explicitly transforming after decoding the avro

[nevillelyh]

In that case I'm not sure this is the best place for that logic. By allowing Set <=> List conversion we're changing the semantics of mapping and records are not guaranteed to round trip, and potentially dangerous, e.g. if the source of truth is Avro array and you mistakenly encoded the field as Set, and loose records as a result.

Can you use a plug-in implicit conversion instead? See AvroType.at[T] in this example:
https://github.com/nevillelyh/shapeless-datatype#avrotype

Also just FYI, the AvroType impl is not the most efficient due to the fact that Avro requires Schema at GenericRecord construction time. It might add significant overhead if you're using this in a high volume application like data pipelines.

[lacarvalho91]

OK I can look at defining our own avro type to do it. Your comment about efficiency seems to be about Avro in general, unless I'm misreading that?

[nevillelyh]

Not it's specifically about this library or shapeless design in general mismatch with Avro.
Other types supported, e.g. BigQuery, JSON, Datastore entity, TF Example are essentially schema-less, and I can dynamically construct records knowing just the Scala type. Not the case for Avro, which requires Schema to be present when constructing GenericRecord, that's why we have AvroRecord & AvroBuilder intermediate types to workaround this issue.

https://github.com/nevillelyh/shapeless-datatype/blob/master/avro/src/main/scala/shapeless/datatype/avro/package.scala#L8
https://github.com/nevillelyh/shapeless-datatype/blob/master/avro/src/main/scala/shapeless/datatype/avro/AvroMappableType.scala#L84

[nevillelyh]

@lacarvalho91 Just checking if you've tried my suggestion?

[lacarvalho91]

Hey @nevillelyh sorry about the delay. Yeah we just defined our own avro type. So I guess this can be closed. Thanks for the help!