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

Fix bug in MapNode #544

Closed
wants to merge 1 commit into from
Closed

Fix bug in MapNode #544

wants to merge 1 commit into from

Conversation

aljoscha
Copy link
Contributor

@aljoscha aljoscha commented Mar 7, 2014

MapNode is responsible for PlainMapOperatorBase and not for
MapOperatorBase, but this is a mess right now.

Maybe we should think about cleaning the situation a bit. CollectorMap is the same as FlatMap, only the function in the interface is called map instead of flatMap. I know that this is because of historic reasons but it is still a mess because we handle all three types through the layers. For example, some code in PactCompiler looks like this:

else if (c instanceof MapOperatorBase) {
    n = new CollectorMapNode((MapOperatorBase<?>) c);
}
else if (c instanceof PlainMapOperatorBase) {
    n = new MapNode((PlainMapOperatorBase<?>) c);
}
else if (c instanceof FlatMapOperatorBase) {
    n = new FlatMapNode((FlatMapOperatorBase<?>) c);
}

MapNode is responsible for PlainMapOperatorBase and not for
MapOperatorBase, but this is a mess right now.
@fhueske
Copy link
Contributor

fhueske commented Mar 7, 2014

I had a fix for this in #542 as well ;-)

@rmetzger rmetzger added the JAPI label Mar 7, 2014
public MapOperatorBase<?> getPactContract() {
return (MapOperatorBase<?>) super.getPactContract();
public PlainMapOperatorBase<?> getPactContract() {
return (PlainMapOperatorBase<?>) super.getPactContract();
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct me if I am wrong, but won't this break with the Record API where Map extends MapOperatorBase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's what I meant by historic reasons. We could change FlatMap and call the method there map() as well...

Copy link
Contributor

Choose a reason for hiding this comment

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

My fix removed the method as it was done before the merge and the build passed Travis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works as well, because MapOperatorBase is converted to a CollectorMapNode, not this MapNode here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright then ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this needs urgent cleanout. I'll try and look over the PRs
soon...

@rmetzger
Copy link
Member

rmetzger commented Mar 7, 2014

Damn.
I suggest to have faster merge cycles for JAPI related (notice the new label) pull requests. As there are more and more developers working on it, we should sort out bugs as fast as possible.

Can we agree to a
2 x +1 (from a committer or @markus-h) AND no complaints ==> merge
voting scheme ?

@fhueske
Copy link
Contributor

fhueske commented Mar 7, 2014

I agree if the two +1 do not include the PR author.

@rmetzger
Copy link
Member

rmetzger commented Mar 7, 2014

Okay. Makes sense.

@fhueske
Copy link
Contributor

fhueske commented Mar 7, 2014

I agree with cleaning up the situation. It also took me quite some time to figure out how the different MapOperators relate to each other...

@fhueske
Copy link
Contributor

fhueske commented Mar 14, 2014

The bug was fixed by another commit.
I'll open a new issue for the clean-up discussion and close this PR.

@fhueske fhueske closed this Mar 14, 2014
@aljoscha aljoscha deleted the fix-map-operator-mess branch July 1, 2014 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants