Add a OrderedSerialization.viaTransform with no dependencies, and a B… #1329
Conversation
…ijectedOrderedSerialization in scalding core
| def viaTransform[T, U]( | ||
| packFn: T => U, | ||
| unpackFn: U => T)(implicit otherOrdSer: OrderedSerialization[U]): OrderedSerialization[T] = | ||
| new OrderedSerialization[T] { |
There was a problem hiding this comment.
Should this be a new type like TransformedOrderedSerialization? Might be useful for optimizing serialization chains, or maybe this is not applicable.
There was a problem hiding this comment.
I'm not sure how we could use that feature right now? we could always add that in later though if we find a use
|
LGTM |
| import com.twitter.bijection.Bijection | ||
|
|
||
| object BijectedOrderedSerialization { | ||
| implicit def fromBijection[T, U](implicit bij: Bijection[T, U], ordSer: OrderedSerialization[U]) = |
There was a problem hiding this comment.
when taking an implicit Bijection, we need to use the ImplicitBijection type because it handles the fact that Bijection[U, T] would also work here.
| import com.twitter.scalding.serialization.OrderedSerialization | ||
| import com.twitter.bijection.ImplicitBijection | ||
|
|
||
| object BijectedOrderedSerialization { |
There was a problem hiding this comment.
Looks good!
Thinking out loud about usage in a scalding job, user will at the very least need to know that she has a type that does not have OrdSer and there is a Bijection from that Type to another Type for which OrdSer does exist and will have to import that bijection. We are slightly moving away from AutoMagically making this work by importing 'RequiredBinaryComparator' but not sure if there is a better way? We should probably improve our error message when we encounter a Type with no OrdSer and nudge user toward this approach.
There was a problem hiding this comment.
Its only ever going to be best effort in the default case I think, if you have a complex JVM type this isn't a good solution either. Though there is a reasonable notion that for new jobs we should force them to re-write in compatible types for the performance improvements... but hard to put in an error message that should write an injection. They probably should write an ordered serialization I suppose... Its a reasonably advanced thing to have to deal with
|
LGTM, merge when green. |
| implicit def fromBijection[T, U](implicit bij: ImplicitBijection[T, U], ordSer: OrderedSerialization[U]) = | ||
| OrderedSerialization.viaTransform[T, U](bij.apply(_), bij.invert(_)) | ||
|
|
||
| implicit def fromInjection[T, U](implicit bij: Injection[T, U], ordSer: OrderedSerialization[U]) = |
There was a problem hiding this comment.
looks like you did not import Injection. Maybe even more coffee? :)
There was a problem hiding this comment.
In my meager defense i got sidetracked and didn't drink the extra coffee yet ;)
|
Fixes #1326 |
…ization Add a OrderedSerialization.viaTransform with no dependencies, and a B…
…ijectedOrderedSerialization in scalding core