Scala implicit ordering fun -- have 2 forms of sketch, defaulting to … #1535
Conversation
| @@ -119,6 +119,67 @@ object TypedPipe extends Serializable { | |||
| override def hash(x: Int): Int = x | |||
| } | |||
| } | |||
| implicit class HigherPriorityTypedPipeMethods[T, K, V](val tp: TypedPipe[T])(implicit ev: TypedPipe[T] <:< TypedPipe[(K, V)], | |||
There was a problem hiding this comment.
Can only enter an implicit class if you meet all of its criteria
There was a problem hiding this comment.
can we drop T entirely here and just do (K, V)?
There was a problem hiding this comment.
Looks like the tests here pass with that yep
There was a problem hiding this comment.
can we just do SketchJoinMethods similar to https://github.com/twitter/scalding/blob/ianoc/addSketchOrderedSerializationInterface/scalding-core/src/main/scala/com/twitter/scalding/typed/TypedPipe.scala#L92 so we can fix (K, V) and simpify a bit
There was a problem hiding this comment.
We can put the classes somewhere else, but I think to keep the same sort of source feel/compatibility we need this low priority implicit stuff to fall back to the old one. If we would be happy to make it disappear in the event of no matching signature i think we can just do 2 implicit methods in the object pointing at 2 classes (in sketched.scala) containing the sketch method of the 2 signatures.
|
looks good to me modulo the naming (I would use names for classes more specific to SketchJoin) |
|
👍 I like this. You can use the OrderedSerialization macros, not have an implicit conversion in scope and do a more efficient join at the same time. Seems like a win. |
|
what do you think of this @rubanm @isnotinvain ? I like not having to set up an implicit conversion and the ability to use compiler generated serializers. |
| .write(TypedText.tsv[(Int, Int, Int)]("output-join")) | ||
| } | ||
|
|
||
| class BothAvailableSketchMethodsTypedSketchLeftJoinJob(args: Args) extends Job(args) { |
There was a problem hiding this comment.
This job looks unused in the test specs below.
|
|
…the existing
@johnynek as we were discussing for the sketch method. More of a consideration than hard PR