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

Generalize handling of merged TypedPipes #975

Merged
merged 2 commits into from
Jul 27, 2014
Merged

Generalize handling of merged TypedPipes #975

merged 2 commits into from
Jul 27, 2014

Conversation

johnynek
Copy link
Collaborator

This flattens merges all the way out into a List, then checks for duplicates in that list. If they exist, do a local self merge leaving us with only distinct pipes, which cascading can handle.

* Cascading can't handle duplicate pipes in merges. What we do here is see if any pipe appears
* multiple times and if it does we can do self merges using flatMap.
* Finally, if there is actually more than one distinct TypedPipe, we use the cascading
* merge primitive.
Copy link
Collaborator

Choose a reason for hiding this comment

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

while in here maybe just mention that we do a rename all the individual pipes -- or are we just assigning names now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right now, we just make an identity pipe (new Pipe(newName, previous)) with a new unique (just integer increment now) name, but only if there is more than one pipe going into the merge.

I'm not totally clear what I should add. Suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a note that we must rename the pipes if we use the merge call since cascading can't handle the multiple pipes possibly with the same name? -- "When using the merge primitive we rename all pipes going into it as Cascading cannot handle multiple pipes with the same name."

@ianoc
Copy link
Collaborator

ianoc commented Jul 25, 2014

LGTM, had a quick question on comment. Not important. Build failure looked a bit spurrious. Restarted. Merge when green with comment change if you think appropriate...fine without too imo.

jcoveney added a commit that referenced this pull request Jul 27, 2014
Generalize handling of merged TypedPipes
@jcoveney jcoveney merged commit 4edd134 into develop Jul 27, 2014
@jcoveney jcoveney deleted the fix_typed_merge branch July 27, 2014 18:10
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.

3 participants