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-16508][SparkR] Split docs for arrange and orderBy methods #14522

Closed
wants to merge 4 commits into from
Closed

[Spark-16508][SparkR] Split docs for arrange and orderBy methods #14522

wants to merge 4 commits into from

Conversation

junyangq
Copy link
Contributor

@junyangq junyangq commented Aug 7, 2016

What changes were proposed in this pull request?

This PR splits arrange and orderBy methods according to their functionality (the former for sorting sparkDataFrame and the latter for windowSpec).

How was this patch tested?

screen shot 2016-08-06 at 6 39 19 pm
screen shot 2016-08-06 at 6 39 29 pm
screen shot 2016-08-06 at 6 40 02 pm

@SparkQA
Copy link

SparkQA commented Aug 7, 2016

Test build #63317 has finished for PR 14522 at commit 0876b75.

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

felixcheung
felixcheung reviewed Aug 7, 2016
View reviewed changes
R/pkg/R/generics.R
@@ -551,7 +551,15 @@ setGeneric("merge")
#' @export
setGeneric("mutate", function(.data, ...) {standardGeneric("mutate") })

#' @rdname arrange
#' Ordering Columns in a WindowSpec
Copy link
Member

Choose a reason for hiding this comment

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

please put this block in windowsspec.R

@felixcheung
Copy link
Member

Could you elaborate how this help with Spark-16508?
Generally we put functions with the same name even if they have different parameters in the same doc - but in this case it seems ok since their behavior are quite different.

@junyangq
Copy link
Contributor Author

junyangq commented Aug 7, 2016 •
edited
Loading

Hi @felixcheung Thanks for reviewing this PR. It happened when I was trying to fix one of the CRAN warnings (Duplicated \argument entries in documentation object arrange: x) and found it confusing to have these two functions in the same doc. After the split, this warning got fixed as well.

@SparkQA
Copy link

SparkQA commented Aug 7, 2016

Test build #63329 has finished for PR 14522 at commit c2445f3.

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

felixcheung
felixcheung reviewed Aug 8, 2016
View reviewed changes
R/pkg/R/WindowSpec.R
#' @param x a WindowSpec
#' @param col a character or Column object indicating an ordering column
#' @param ... additional sorting fields
#' @return A WindowSpec
Copy link
Member

Choose a reason for hiding this comment

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

lower case 'a' to be consistent?

@SparkQA
Copy link

SparkQA commented Aug 8, 2016

Test build #63368 has finished for PR 14522 at commit f811a7d.

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

felixcheung
felixcheung reviewed Aug 9, 2016
View reviewed changes
R/pkg/R/generics.R
@@ -771,6 +771,10 @@ setGeneric("over", function(x, window) { standardGeneric("over") })

###################### WindowSpec Methods ##########################

#' @rdname orderBy
#' @export
Copy link
Member

Choose a reason for hiding this comment

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

hmm, that's interesting. I'm not sure if this is necessary, or if this works. I think the purpose of setGeneric would only need this once. By putting a @Rdname tag it will show up on the Rd file, but I don't think we need to put it twice, once in different Rd?

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 checked the generated docs, and it showed in both docs, but I do agree that it is not necessary considering the purpose of setGeneric. Then we have to decide where to put 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.

Perhaps to orderBy? When applied to SparkDataFrame, it simply calls the arrange function.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's ok. Doc for the generics isn't super useful anyway.

@SparkQA
Copy link

SparkQA commented Aug 10, 2016

Test build #63549 has finished for PR 14522 at commit a52256b.

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

@felixcheung
Copy link
Member

LGTM. Minor point about @name - though it will be good to know since we might tell contributors to always add that

@felixcheung
Copy link
Member

Since this fixes some CRAN check warnings, let's merge this? @shivaram what do you think?

@shivaram
Copy link
Contributor

Yeah LGTM. Merging this to master, branch-2.0 -- Thanks @junyangq

@asfgit asfgit closed this in 564fe61 Aug 15, 2016
asfgit pushed a commit that referenced this pull request Aug 15, 2016
@junyangq @shivaram
[SPARK-16508][SPARKR] Split docs for arrange and orderBy methods
8f4cacd 
This PR splits arrange and orderBy methods according to their functionality (the former for sorting sparkDataFrame and the latter for windowSpec).

![screen shot 2016-08-06 at 6 39 19 pm](https://cloud.githubusercontent.com/assets/15318264/17459969/51eade28-5c05-11e6-8ca1-8d8a8e344bab.png)
![screen shot 2016-08-06 at 6 39 29 pm](https://cloud.githubusercontent.com/assets/15318264/17459966/51e3c246-5c05-11e6-8d35-3e905ca48676.png)
![screen shot 2016-08-06 at 6 40 02 pm](https://cloud.githubusercontent.com/assets/15318264/17459967/51e650ec-5c05-11e6-8698-0f037f5199ff.png)

Author: Junyang Qian <[email protected]>

Closes #14522 from junyangq/SPARK-16508-0.

(cherry picked from commit 564fe61)
Signed-off-by: Shivaram Venkataraman <[email protected]>
ColZer pushed a commit to ColZer/spark that referenced this pull request Aug 16, 2016
@junyangq @ColZer
[SPARK-16508][SPARKR] Split docs for arrange and orderBy methods
75db5a9 
## What changes were proposed in this pull request?

This PR splits arrange and orderBy methods according to their functionality (the former for sorting sparkDataFrame and the latter for windowSpec).

## How was this patch tested?

![screen shot 2016-08-06 at 6 39 19 pm](https://cloud.githubusercontent.com/assets/15318264/17459969/51eade28-5c05-11e6-8ca1-8d8a8e344bab.png)
![screen shot 2016-08-06 at 6 39 29 pm](https://cloud.githubusercontent.com/assets/15318264/17459966/51e3c246-5c05-11e6-8d35-3e905ca48676.png)
![screen shot 2016-08-06 at 6 40 02 pm](https://cloud.githubusercontent.com/assets/15318264/17459967/51e650ec-5c05-11e6-8698-0f037f5199ff.png)

Author: Junyang Qian <[email protected]>

Closes apache#14522 from junyangq/SPARK-16508-0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@junyangq @SparkQA @felixcheung @shivaram