Skip to content

Conversation

@felixcheung
Copy link
Member

@felixcheung felixcheung commented Jun 21, 2016

What changes were proposed in this pull request?

I ran a full pass from A to Z and fixed the obvious duplications, improper grouping etc.

There are still more doc issues to be cleaned up.

How was this patch tested?

manual tests

@felixcheung
Copy link
Member Author

@shivaram @dongjoon-hyun

@felixcheung felixcheung changed the title [SPARKR][DOCS] R code doc cleanup [SPARK-16088][SPARKR][DOCS] Remove setJobGroup, clearJobGroup, cancelJobGroup, R code doc cleanup Jun 21, 2016
@felixcheung
Copy link
Member Author

Opened a JIRA since we are removing methods.

@SparkQA
Copy link

SparkQA commented Jun 21, 2016

Test build #60903 has finished for PR 13798 at commit 345b9e2.

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

@shivaram
Copy link
Contributor

Hmm can we document somewhere why we want to remove this functionality ? Is this because spark context is no longer accessible ?

@felixcheung
Copy link
Member Author

Right they are really SparkContext methods - I could leave/deprecate them if you'd prefer, or separate that into a different PR?

@felixcheung felixcheung changed the title [SPARK-16088][SPARKR][DOCS] Remove setJobGroup, clearJobGroup, cancelJobGroup, R code doc cleanup [SPARKR][DOCS] R code doc cleanup Jun 21, 2016
@felixcheung
Copy link
Member Author

No more jobGroup changes

@shivaram
Copy link
Contributor

Yeah lets do that in a separate PR with discussion ? We don't lose much by leaving in a few extra functions that we deprecate / remove later.

Thanks for the doc cleanup - I'll take one more look at that now

setGeneric("isNull", function(x) { standardGeneric("isNull") })

#' @rdname column
#' @rdname column functions
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure i follow the intent here - do we want these all in the same rd ? Also is there supposed to be an underscore (i.e. column_functions) instead of a space ?

Copy link
Member Author

Choose a reason for hiding this comment

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

They were on one page, but "column" is the wrong rd to put them - there's already the class Column and function column. (there's also function columns but that's a different page) - so the intent is to put these in their separate pages as column functions)

it looked like this previously:
image

This is still buggy - investigating.

@SparkQA
Copy link

SparkQA commented Jun 21, 2016

Test build #60906 has finished for PR 13798 at commit 28916fc.

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

@felixcheung
Copy link
Member Author

Fixed. I double-checked all affected rd.

#' \item{"Option 3"} {Return a new SparkDataFrame partitioned by the given column(s),
#' \item{2.} {Return a new SparkDataFrame that has exactly `numPartitions`.}
#' \item{3.} {Return a new SparkDataFrame partitioned by the given column(s),
#' using `spark.sql.shuffle.partitions` as number of partitions.}
Copy link
Member

Choose a reason for hiding this comment

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

For the ordered itemize, what about the following?

#' \enumerate{
#'  \item Return a new SparkDataFrame partitioned by the given columns into `numPartitions`.
#'  \item Return a new SparkDataFrame that has exactly `numPartitions`.
#'  \item Return a new SparkDataFrame partitioned by the given column(s),
#'                     using `spark.sql.shuffle.partitions` as number of partitions.
#' }

Copy link
Member Author

Choose a reason for hiding this comment

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

it mentioned "these options" (as in one of these choices) on the line before, so I thought it's nicer to numbered the options.
in other cases we have unnumbered bullets if they are enum values and so on.

@shivaram
Copy link
Contributor

Thanks for the update. This one looks fine to me now.

@dongjoon-hyun Any other comments ?

@SparkQA
Copy link

SparkQA commented Jun 21, 2016

Test build #60909 has finished for PR 13798 at commit d9d9b3e.

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

@shivaram
Copy link
Contributor

Alright I'm merging this to master and branch-2.0 so that it makes it to the RC. We can try and fix minor things going forward

Thanks @felixcheung -- This is a much needed clean up.

asfgit pushed a commit that referenced this pull request Jun 21, 2016
## What changes were proposed in this pull request?

I ran a full pass from A to Z and fixed the obvious duplications, improper grouping etc.

There are still more doc issues to be cleaned up.

## How was this patch tested?

manual tests

Author: Felix Cheung <[email protected]>

Closes #13798 from felixcheung/rdocseealso.

(cherry picked from commit 09f4cea)
Signed-off-by: Shivaram Venkataraman <[email protected]>
@asfgit asfgit closed this in 09f4cea Jun 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants