You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@datafu.apache.org by Jason Reid <ja...@stingrayvbc.com> on 2014/10/14 19:49:30 UTC
Review Request 26694: DATAFU-70: Add BagFullOuterJoin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26694/
-----------------------------------------------------------
Review request for DataFu.
Repository: datafu
Description
-------
DATAFU-70: Add BagFullOuterJoin
Diffs
-----
datafu-pig/src/main/java/datafu/pig/bags/BagJoin.java PRE-CREATION
datafu-pig/src/main/java/datafu/pig/bags/BagLeftOuterJoin.java ba6bc11
datafu-pig/src/test/java/datafu/test/pig/bags/BagTests.java 6d21dbe
Diff: https://reviews.apache.org/r/26694/diff/
Testing
-------
Added bagJoinFullOuterTest to BagTests which uses the same input as the bagLeftOuterJoinTest but asserts the full outer join logic in the output. I have also refactored the existing BagLeftOuterJoin class to use the generic BagJoin class and the existing test continues to pass without modification.
Thanks,
Jason Reid
Re: Review Request 26694: DATAFU-70: Add BagFullOuterJoin
Posted by Matthew Hayes <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26694/#review57273
-----------------------------------------------------------
This looks pretty good to me overall, thanks for doing this! I just have some minor comments.
datafu-pig/src/main/java/datafu/pig/bags/BagJoin.java
<https://reviews.apache.org/r/26694/#comment97829>
Doc not accurate
datafu-pig/src/main/java/datafu/pig/bags/BagJoin.java
<https://reviews.apache.org/r/26694/#comment97822>
I think "full" would be better than "outer". Thoughts?
datafu-pig/src/main/java/datafu/pig/bags/BagJoin.java
<https://reviews.apache.org/r/26694/#comment97824>
An enum with values LEFT and FULL would be clearer than two flags probably.
datafu-pig/src/main/java/datafu/pig/bags/BagJoin.java
<https://reviews.apache.org/r/26694/#comment97828>
comment not accurate
- Matthew Hayes
On Oct. 14, 2014, 5:49 p.m., Jason Reid wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26694/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2014, 5:49 p.m.)
>
>
> Review request for DataFu.
>
>
> Repository: datafu
>
>
> Description
> -------
>
> DATAFU-70: Add BagFullOuterJoin
>
>
> Diffs
> -----
>
> datafu-pig/src/main/java/datafu/pig/bags/BagJoin.java PRE-CREATION
> datafu-pig/src/main/java/datafu/pig/bags/BagLeftOuterJoin.java ba6bc11
> datafu-pig/src/test/java/datafu/test/pig/bags/BagTests.java 6d21dbe
>
> Diff: https://reviews.apache.org/r/26694/diff/
>
>
> Testing
> -------
>
> Added bagJoinFullOuterTest to BagTests which uses the same input as the bagLeftOuterJoinTest but asserts the full outer join logic in the output. I have also refactored the existing BagLeftOuterJoin class to use the generic BagJoin class and the existing test continues to pass without modification.
>
>
> Thanks,
>
> Jason Reid
>
>
Re: Review Request 26694: DATAFU-70: Add BagFullOuterJoin
Posted by Matthew Hayes <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26694/#review62523
-----------------------------------------------------------
Ship it!
Thanks for fixing this! The changes look good. I have some minor improvements that I'll make in a subsequent commit, basically: update the constructor and add an inner join test.
- Matthew Hayes
On Oct. 20, 2014, 7 p.m., Jason Reid wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26694/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2014, 7 p.m.)
>
>
> Review request for DataFu.
>
>
> Repository: datafu
>
>
> Description
> -------
>
> DATAFU-70: Add BagFullOuterJoin
>
>
> Diffs
> -----
>
> datafu-pig/src/main/java/datafu/pig/bags/BagJoin.java PRE-CREATION
> datafu-pig/src/main/java/datafu/pig/bags/BagLeftOuterJoin.java ba6bc11
> datafu-pig/src/test/java/datafu/test/pig/bags/BagTests.java 6d21dbe
>
> Diff: https://reviews.apache.org/r/26694/diff/
>
>
> Testing
> -------
>
> Added bagJoinFullOuterTest to BagTests which uses the same input as the bagLeftOuterJoinTest but asserts the full outer join logic in the output. I have also refactored the existing BagLeftOuterJoin class to use the generic BagJoin class and the existing test continues to pass without modification.
>
>
> Thanks,
>
> Jason Reid
>
>
Re: Review Request 26694: DATAFU-70: Add BagFullOuterJoin
Posted by Jason Reid <ja...@stingrayvbc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26694/
-----------------------------------------------------------
(Updated Oct. 20, 2014, 7 p.m.)
Review request for DataFu.
Changes
-------
Updated to address comments from v1.
Repository: datafu
Description
-------
DATAFU-70: Add BagFullOuterJoin
Diffs (updated)
-----
datafu-pig/src/main/java/datafu/pig/bags/BagJoin.java PRE-CREATION
datafu-pig/src/main/java/datafu/pig/bags/BagLeftOuterJoin.java ba6bc11
datafu-pig/src/test/java/datafu/test/pig/bags/BagTests.java 6d21dbe
Diff: https://reviews.apache.org/r/26694/diff/
Testing
-------
Added bagJoinFullOuterTest to BagTests which uses the same input as the bagLeftOuterJoinTest but asserts the full outer join logic in the output. I have also refactored the existing BagLeftOuterJoin class to use the generic BagJoin class and the existing test continues to pass without modification.
Thanks,
Jason Reid