[FLINK-2106] [runtime] add Left-, Right- and Full Outer Join drivers #1052

r-pogalz · 2015-08-24T22:15:08Z

This PR includes the implementation of 3 drivers for the specific outer joins and adapts the DriverStrategy for the merge based outer joins.

To make the outer join operators implemented in ticket FLINK-2105 visible to the Java and Scala API, we have started with the integration of the merge based outer join operators bottom up. To reduce duplicated code we implemented an AbstractOuterJoinDriver which is responsible for most parts of the drivers setup and running the operators. The specific outer join driver implementations (LeftOuterJoinDriver, RightOuterJoinDriver, FullOuterJoinDriver) set the corresponding Outer Join Iterators based on the ExecutionConfig. Therefore, we had to adapt the DriverStrategy adding three additional enum fields (LEFT_OUTER_MERGE, RIGHT_OUTER_MERGE, FULL_OUTER_MERGE).
Currently, we only support the outer join on a merged based strategy, but hash based will follow (FLINK-2107).

Furthermore, DriverStrategy.MERGE has been renamed to DriverStrategy.INNER_MERGE as requested by @fhueske.

Next steps of this issue will be the optimizer and Java/Scala API parts.

chiwanpark · 2015-08-25T02:47:52Z

Hi, Thanks for your contribution. I'll review this and leave comments.

tillrohrmann · 2015-08-25T08:24:47Z

Thanks for your contribution @r-pogalz. Could you please add some more details about the implementation to the PR description.

tillrohrmann · 2015-08-25T08:27:57Z

flink-runtime/src/main/java/org/apache/flink/runtime/operators/AbstractOuterJoinDriver.java

+		final TypeComparator<IT2> comparator2 = this.taskContext.getDriverComparator(1);
+
+		final TypePairComparatorFactory<IT1, IT2> pairComparatorFactory = config.getPairComparatorFactory(
+				this.taskContext.getUserCodeClassLoader());


tillrohrmann · 2015-08-25T12:18:50Z

Really good work @r-pogalz. I had only some minor comments concerning style and test cases.

I like your approach to split the implementation of FLINK-687 into multiple parts. This makes it far easier to review. Concerning the description of FLINK-2106, you haven't integrated the outer sort merge join into the optimizer and the API, yet. I guess this will happen as a next step. Maybe you can update the description of FLINK-2106 accordingly.

Other than that, the PR looks good to me :-)

r-pogalz · 2015-08-26T08:38:11Z

Hi @tillrohrmann, thanks for your review and comments. I will work through it soon.

Regarding the ticket FLINK-2106, @fhueske suggested us to create multiple PRs for runtime, optimizer and API integration. I guess it is an unusual way, but is it possible to have more than one PR for an issue? If not, I will change the description of FLINK-2106 as you said and create two more tasks for the optimizer and API part.

tillrohrmann · 2015-08-26T08:49:01Z

Hi @r-pogalz, usually you only have one PR for a ticket. Thus, it would be good to create two new sub-tasks for the API and the optimizer integration.

fhueske · 2015-08-26T08:52:32Z

@r-pogalz I would implement API and optimizer together in one issue. IMO it is easier to start from the API and then work your way through API to optimizer and finally use your runtime code. However, you cannot add the API without adding the optimizer part. So, I would do both in one issue.

fhueske · 2015-08-26T13:17:13Z

flink-runtime/src/main/java/org/apache/flink/runtime/operators/AbstractOuterJoinDriver.java

+		final double fractionAvailableMemory = config.getRelativeMemoryDriver();
+		final int numPages = memoryManager.computeNumberOfPages(fractionAvailableMemory);
+
+		// test minimum memory requirements


unrelated comment

r-pogalz · 2015-08-29T22:03:02Z

Hi @tillrohrmann, @fhueske and @chiwanpark,
thanks for your reviews and comments. I went through the classes and changed the formatting. Moreover, I refactored the tests so that exceptions are not hidden somewhere on the console anymore and changed the testCancel*() methods such that the inputs are infinite now to prevent the driver finishing before cancel() is called.

fhueske · 2015-09-03T14:51:31Z

flink-runtime/src/test/java/org/apache/flink/runtime/operators/AbstractOuterJoinTaskTest.java

+		addInput(new UniformIntTupleGenerator(keyCnt2, valCnt2, true), this.serializer);
+
+		testDriver(testTask, MockFailingJoinStub.class);
+		Assert.fail("Driver did not forward Exception.");


You can remove this assertion.
The test will fail if the test method does not throw an ExpectedTestException.

fhueske · 2015-09-03T15:20:18Z

Hi @r-pogalz, @jkovacs,

thanks for the update. It's good to merge, IMO.
Either you resolve my minor comments or I can do it before merging.

Anybody else whats to have a look at this PR?

chiwanpark · 2015-09-04T09:41:01Z

Looks good to merge. 👍

StephanEwen · 2015-09-04T14:53:20Z

I just saw that the OuterJoin runtime classes/tests cause a lot of warnings when compiling. Manyly raw types, unchecked casts.

Would be good to fix those. In general, we strive for as little warnings as possible. Generic warnings should be fixed or suppressed (if impossible to fix). Otherwise they crowd the warnings output so that the actual critical warnings get lost.

…ies to Runtime

…INNER_MERGE

r-pogalz · 2015-09-04T16:15:42Z

Thanks for you comment @StephanEwen, good to know that. I tried to minimize the occurrences of warnings where possible by fixing or suppressing it for this issue. Should I also fix it for the previous issue (runtime OuterJoin operators and tests) and commit it together with this one?

StephanEwen · 2015-09-04T16:20:37Z

Yeah, adding warnings fix to this PR is good.

…n iterators and tests

r-pogalz · 2015-09-07T08:02:57Z

The warnings fix commit is ready for review.

fhueske · 2015-09-07T16:37:29Z

Looks good to me. Will merge this tomorrow, if nobody objects.
Thanks @r-pogalz and @jkovacs!

StephanEwen · 2015-09-08T09:14:41Z

I did not go through all details, but from what I read it was good. Go ahead from my side...

fhueske · 2015-09-08T13:59:35Z

Will merge this now

fhueske · 2015-09-08T15:05:44Z

Thanks @r-pogalz and @jkovacs!

…INNER_MERGE This closes apache#1052

r-pogalz changed the title ~~[FLINK-2106] Runtime part~~ [FLINK-2106] [runtime] add Left-, Right- and Full Outer Join drivers Aug 24, 2015

tillrohrmann reviewed Aug 25, 2015
View reviewed changes

fhueske reviewed Aug 26, 2015
View reviewed changes

r-pogalz force-pushed the feature/FLINK-2106-Runtime branch 2 times, most recently from a1331e3 to 46f825e Compare August 29, 2015 21:39

fhueske reviewed Sep 3, 2015
View reviewed changes

r-pogalz force-pushed the feature/FLINK-2106-Runtime branch from 46f825e to 0cfae3a Compare September 3, 2015 17:11

r-pogalz added 2 commits September 4, 2015 17:59

[FLINK-2106] [runtime] Add Outer Join drivers and Outer Merge strateg…

17dd0c0

…ies to Runtime

[FLINK-2106] [runtime] Rename DriverStrategy.MERGE to DriverStrategy.…

9e76ec9

…INNER_MERGE

r-pogalz force-pushed the feature/FLINK-2106-Runtime branch from 0cfae3a to 9e76ec9 Compare September 4, 2015 16:00

[FLINK-2106] [runtime] Fix/suppress compilation warnings for OuterJoi…

9064e2f

…n iterators and tests

r-pogalz force-pushed the feature/FLINK-2106-Runtime branch from 29f95ec to 9064e2f Compare September 5, 2015 11:01

asfgit closed this in 2da82f9 Sep 8, 2015

jkovacs mentioned this pull request Sep 16, 2015

[FLINK-2576] Add Outer Join operator to Optimizer and APIs #1138

Closed

nikste pushed a commit to nikste/flink that referenced this pull request Sep 29, 2015

[FLINK-2106] [runtime] Rename DriverStrategy.MERGE to DriverStrategy.…

60a1ddc

…INNER_MERGE This closes apache#1052

rmetzger added the component=Runtime/Operators label Mar 14, 2019

flinkbot added component=Runtime/Task and removed component=Runtime/Operators labels Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-2106] [runtime] add Left-, Right- and Full Outer Join drivers #1052

[FLINK-2106] [runtime] add Left-, Right- and Full Outer Join drivers #1052

r-pogalz commented Aug 24, 2015

chiwanpark commented Aug 25, 2015

tillrohrmann commented Aug 25, 2015

tillrohrmann Aug 25, 2015

tillrohrmann commented Aug 25, 2015

r-pogalz commented Aug 26, 2015

tillrohrmann commented Aug 26, 2015

fhueske commented Aug 26, 2015

fhueske Aug 26, 2015

r-pogalz commented Aug 29, 2015

fhueske Sep 3, 2015

fhueske commented Sep 3, 2015

chiwanpark commented Sep 4, 2015

StephanEwen commented Sep 4, 2015

r-pogalz commented Sep 4, 2015

StephanEwen commented Sep 4, 2015

r-pogalz commented Sep 7, 2015

fhueske commented Sep 7, 2015

StephanEwen commented Sep 8, 2015

fhueske commented Sep 8, 2015

fhueske commented Sep 8, 2015

[FLINK-2106] [runtime] add Left-, Right- and Full Outer Join drivers #1052

[FLINK-2106] [runtime] add Left-, Right- and Full Outer Join drivers #1052

Conversation

r-pogalz commented Aug 24, 2015

chiwanpark commented Aug 25, 2015

tillrohrmann commented Aug 25, 2015

tillrohrmann Aug 25, 2015

Choose a reason for hiding this comment

tillrohrmann commented Aug 25, 2015

r-pogalz commented Aug 26, 2015

tillrohrmann commented Aug 26, 2015

fhueske commented Aug 26, 2015

fhueske Aug 26, 2015

Choose a reason for hiding this comment

r-pogalz commented Aug 29, 2015

fhueske Sep 3, 2015

Choose a reason for hiding this comment

fhueske commented Sep 3, 2015

chiwanpark commented Sep 4, 2015

StephanEwen commented Sep 4, 2015

r-pogalz commented Sep 4, 2015

StephanEwen commented Sep 4, 2015

r-pogalz commented Sep 7, 2015

fhueske commented Sep 7, 2015

StephanEwen commented Sep 8, 2015

fhueske commented Sep 8, 2015

fhueske commented Sep 8, 2015