-
Notifications
You must be signed in to change notification settings - Fork 705
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 pipe1.join(pipe2) syntax in TypedAPI #958
Conversation
* This is a class that models the logical portion of the reduce step. | ||
* in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woops.
This also seems safe to me. |
@@ -660,13 +677,6 @@ final case class MergedTypedPipe[T](left: TypedPipe[T], right: TypedPipe[T]) ext | |||
MergedTypedPipe(left.hashCogroup(smaller)(joiner), right.hashCogroup(smaller)(joiner)) | |||
} | |||
|
|||
class TuplePipeJoinEnrichment[K, V](pipe: TypedPipe[(K, V)])(implicit ord: Ordering[K]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this before was .join not possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do this enrichment, but you had to explicitly import it. That is another way to go, just make this class what we enrich to. It more narrowly constrains the enrichment.
That said, it seemed like HashJoinable was the right place to jump in, since we also want pipe1.join(pipe2).join(pipe2)
type syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, fair enough. it also moves to the newer .withReducers code paths too.
LGTM |
Merge when green |
Looks great. |
Add pipe1.join(pipe2) syntax in TypedAPI
@vitalyg (and others) have argued that doing:
pipe1.group.join(pipe2.group)
is quite ugly. This change adds an implicit toHashJoinable
which is a subclass ofCoGroupable
. This means that you can join(K, V)
pipes without any extra implicit.This seems safe since the only methods added by the enrichment are the join ones (not the reducing methods on KeyedListLike).
Please speak up if this looks at all dangerous (as implicit conversions can be when poorly designed).