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.

Sign up for GitHub

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

Jump to bottom

[SPARK-19126][Docs] Update Join Documentation Across Languages #16504

Closed
wants to merge 5 commits into from
Closed

[SPARK-19126][Docs] Update Join Documentation Across Languages #16504

wants to merge 5 commits into from

Conversation

bllchmbrs
Copy link
Contributor

@bllchmbrs bllchmbrs commented Jan 8, 2017 •
edited
Loading

What changes were proposed in this pull request?

  • Make sure all join types are clearly mentioned
  • Make join labeling/style consistent
  • Make join label ordering docs the same
  • Improve join documentation according to above for Scala
  • Improve join documentation according to above for Python
  • Improve join documentation according to above for R

How was this patch tested?

No tests b/c docs.

Please review http://spark.apache.org/contributing.html before opening a pull request.

@bllchmbrs bllchmbrs changed the title [SPAKR-19126][Docs] Update Join Documentation Across Languages [SPARK-19126][Docs] Update Join Documentation Across Languages Jan 8, 2017
@SparkQA
Copy link

SparkQA commented Jan 8, 2017

Test build #71042 has finished for PR 16504 at commit fe27b0c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

felixcheung
felixcheung reviewed Jan 8, 2017
View reviewed changes
R/pkg/R/DataFrame.R
#' 'right_outer', 'rightouter', 'right', and 'leftsemi'. The default joinType is "inner".
#' @param joinType The type of join to perform, default 'inner'.
#' Must be any of: 'inner', 'cross', 'outer', 'full', 'full_outer',
#' 'left', 'left_outer', 'right', 'right_outer', 'left_semi', and 'left_anti'.
Copy link
Member

@felixcheung felixcheung Jan 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

this isn't really quite matching the code -
we need to add cross, full_outer, left_anti, left_semi to L2347 and L2354

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, didn't know those were there. Added them now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

new changes should be good, let me know!

felixcheung
felixcheung reviewed Jan 8, 2017
View reviewed changes
python/pyspark/sql/dataframe.py
@@ -730,8 +730,9 @@ def join(self, other, on=None, how=None):
a join expression (Column), or a list of Columns.
If `on` is a string or a list of strings indicating the name of the join column(s),
the column(s) must exist on both sides, and this performs an equi-join.
:param how: str, default 'inner'.
One of `inner`, `outer`, `left_outer`, `right_outer`, `leftsemi`.
:param how: str, default ``inner``. Must be any of: ``inner``, ``cross``, ``outer``,
Copy link
Member

Choose a reason for hiding this comment

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

perhaps Must be one of is more concise and consistent?

Copy link
Member

Choose a reason for hiding this comment

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

ditto in other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really sure I agree with either or those. It's concise because both "must be one of" and "must be any of" are the same number of characters and words. It's consistent because that's what I used everywhere and there are virtually no instances of "must be one of" in Spark except in some random examples and docs.

With all that being said, I guess it is a bit strange grammatically and I am happy to change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

Choose a reason for hiding this comment

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

agreed, I'd say the differences are minor

@felixcheung
Copy link
Member

LGTM.

@SparkQA
Copy link

SparkQA commented Jan 9, 2017

Test build #71046 has finished for PR 16504 at commit 61b6c07.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@bllchmbrs
Copy link
Contributor Author

jenkins test

@SparkQA
Copy link

SparkQA commented Jan 9, 2017

Test build #71048 has finished for PR 16504 at commit 66b7180.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@felixcheung
Copy link
Member

felixcheung commented Jan 9, 2017 •
edited
Loading

@anabranch the failure is not from Jenkins but from (R changes triggered) AppVeyor tests, to run on Windows. It looks like it is nothing related - it failed at access github.

And the earlier iteration passed.

asfgit pushed a commit that referenced this pull request Jan 9, 2017
@bllchmbrs
[SPARK-19126][DOCS] Update Join Documentation Across Languages
8779e6a 
## What changes were proposed in this pull request?

- [X] Make sure all join types are clearly mentioned
- [X] Make join labeling/style consistent
- [X] Make join label ordering docs the same
- [X] Improve join documentation according to above for Scala
- [X] Improve join documentation according to above for Python
- [X] Improve join documentation according to above for R

## How was this patch tested?
No tests b/c docs.

Please review http://spark.apache.org/contributing.html before opening a pull request.

Author: anabranch <[email protected]>

Closes #16504 from anabranch/SPARK-19126.

(cherry picked from commit 19d9d4c)
Signed-off-by: Felix Cheung <[email protected]>
@felixcheung
Copy link
Member

merged to master & branch-2.1

@asfgit asfgit closed this in 19d9d4c Jan 9, 2017
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
@bllchmbrs @uzadude
[SPARK-19126][DOCS] Update Join Documentation Across Languages
a88abd8 
## What changes were proposed in this pull request?

- [X] Make sure all join types are clearly mentioned
- [X] Make join labeling/style consistent
- [X] Make join label ordering docs the same
- [X] Improve join documentation according to above for Scala
- [X] Improve join documentation according to above for Python
- [X] Improve join documentation according to above for R

## How was this patch tested?
No tests b/c docs.

Please review http://spark.apache.org/contributing.html before opening a pull request.

Author: anabranch <[email protected]>

Closes apache#16504 from anabranch/SPARK-19126.
cmonkey pushed a commit to cmonkey/spark that referenced this pull request Feb 15, 2017
@bllchmbrs @cmonkey
[SPARK-19126][DOCS] Update Join Documentation Across Languages
f194ac7 
## What changes were proposed in this pull request?

- [X] Make sure all join types are clearly mentioned
- [X] Make join labeling/style consistent
- [X] Make join label ordering docs the same
- [X] Improve join documentation according to above for Scala
- [X] Improve join documentation according to above for Python
- [X] Improve join documentation according to above for R

## How was this patch tested?
No tests b/c docs.

Please review http://spark.apache.org/contributing.html before opening a pull request.

Author: anabranch <[email protected]>

Closes apache#16504 from anabranch/SPARK-19126.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixcheung felixcheung felixcheung left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bllchmbrs @SparkQA @felixcheung