Skip to content

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

Closed
bllchmbrs wants to merge 5 commits into
apache:masterfrom
bllchmbrs:SPARK-19126
Closed

[SPARK-19126][Docs] Update Join Documentation Across Languages#16504
bllchmbrs wants to merge 5 commits into
apache:masterfrom
bllchmbrs:SPARK-19126

Conversation

@bllchmbrs
Copy link
Copy Markdown
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
Copy Markdown

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
Comment thread R/pkg/R/DataFrame.R Outdated
#' '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
Copy Markdown
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
Copy Markdown
Contributor Author

@bllchmbrs bllchmbrs Jan 8, 2017

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
Copy Markdown
Contributor Author

@bllchmbrs bllchmbrs Jan 8, 2017

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
Comment thread python/pyspark/sql/dataframe.py Outdated
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
Copy Markdown
Member

@felixcheung felixcheung Jan 8, 2017

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
Copy Markdown
Member

@felixcheung felixcheung Jan 8, 2017

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
Copy Markdown
Contributor Author

@bllchmbrs bllchmbrs Jan 9, 2017

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
Copy Markdown
Contributor Author

@bllchmbrs bllchmbrs Jan 9, 2017

Choose a reason for hiding this comment

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

done.

Copy link
Copy Markdown
Member

@felixcheung felixcheung Jan 9, 2017

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
Copy Markdown
Member

felixcheung commented Jan 8, 2017

LGTM.

@SparkQA
Copy link
Copy Markdown

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
Copy Markdown
Contributor Author

bllchmbrs commented Jan 9, 2017

jenkins test

@SparkQA
Copy link
Copy Markdown

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
Copy Markdown
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 <wac.chambers@gmail.com>

Closes #16504 from anabranch/SPARK-19126.

(cherry picked from commit 19d9d4c)
Signed-off-by: Felix Cheung <felixcheung@apache.org>
@felixcheung
Copy link
Copy Markdown
Member

felixcheung commented Jan 9, 2017

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 <wac.chambers@gmail.com>

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 <wac.chambers@gmail.com>

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