[SPARK-20889][SparkR] Grouped documentation for DATETIME column methods #18114

actuaryzhang · 2017-05-25T23:03:18Z

What changes were proposed in this pull request?

Grouped documentation for datetime column methods.

actuaryzhang · 2017-05-25T23:08:13Z

@felixcheung
Created this PR to update the doc for the date time methods, similar to #18025. About 27 date time methods are documented into one page.
I'm attaching the snapshot of part of the new help page.

actuaryzhang · 2017-05-25T23:09:41Z

R/pkg/R/functions.R

-            if (class(x) == "Column") {
-              x <- x@jc
+setMethod("datediff", signature(x = "Column"),
+          function(x, y) {


Here, x and y are reversed for easy documentation. Similarly for other methods that take two arguments.

actuaryzhang · 2017-05-25T23:11:08Z

R/pkg/R/functions.R

-          function(y, x) {
-            jc <- callJStatic("org.apache.spark.sql.functions", "from_utc_timestamp", y@jc, x)
+setMethod("from_utc_timestamp", signature(x = "Column", tz = "character"),
+          function(x, tz) {


Changed the second argument to tz to be consistent with Scala, which also makes it less confusing in the doc since other methods also have y as argument that often refers to a Column.

ditto here on calling by name

SparkQA · 2017-05-25T23:41:12Z

Test build #77391 has finished for PR 18114 at commit 0d2853d.

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

felixcheung · 2017-05-26T05:20:14Z

R/pkg/R/functions.R

-#' Returns the number of days from \code{start} to \code{end}.
-#'
-#' @param x start Column to use.
-#' @param y end Column to use.


two concerns here:

the doc description is now fairly generalized - is supposed to be datediff(end, start) but seems like it's not not clear which is end and which is start

renaming this could break user calling by name datediff(df$c, x = "foo")

@felixcheung These names start and end are from the original doc. I now changed it to x and y.

felixcheung · 2017-05-26T05:20:51Z

R/pkg/R/functions.R

-          function(y, x) {
-            jc <- callJStatic("org.apache.spark.sql.functions", "from_utc_timestamp", y@jc, x)
+setMethod("from_utc_timestamp", signature(x = "Column", tz = "character"),
+          function(x, tz) {


ditto here on calling by name

felixcheung · 2017-05-26T05:23:43Z

maybe it's worthwhile to separate the dff type functions into a separate rd so we don't have to rename/switch the parameter?

actuaryzhang · 2017-05-26T05:57:18Z

@felixcheung Thank you. This is great suggestion. I will split it into two help files which should make the doc much cleaner without changing the functions.

actuaryzhang · 2017-05-26T23:10:10Z

@felixcheung The new commit addresses your concern by splitting methods with two arguments into a separate doc.

SparkQA · 2017-05-26T23:14:47Z

Test build #77441 has finished for PR 18114 at commit 944aa92.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
`#' @param x if of class Column, it is used to perform arithmatic operations with

felixcheung · 2017-05-27T03:47:08Z

Jenkins, retest this please

SparkQA · 2017-05-27T04:28:20Z

Test build #77447 has finished for PR 18114 at commit 944aa92.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
`#' @param x if of class Column, it is used to perform arithmatic operations with

SparkQA · 2017-05-27T06:47:36Z

Test build #77452 has started for PR 18114 at commit 016bb47.

actuaryzhang · 2017-05-27T08:05:53Z

Jenkins, retest this please

SparkQA · 2017-05-27T08:41:11Z

Test build #77459 has finished for PR 18114 at commit 016bb47.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
`#' @param x For class Column, it is used to perform arithmetic operations with

actuaryzhang · 2017-06-20T03:39:52Z

For the date time functions, I create two groups: one for arithmetic functions that work with two columns column_datetime_diff_functions, and the other for functions that work with only one column column_datetime_functions. Below is the screenshot for both.

actuaryzhang · 2017-06-20T03:41:11Z

For the column_datetime_diff_functions:

SparkQA · 2017-06-20T04:10:53Z

Test build #78271 has finished for PR 18114 at commit 311ccc2.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
`#' For class numeric, it is the number of months or days to be added to

SparkQA · 2017-06-20T05:41:06Z

Test build #78272 has finished for PR 18114 at commit aab9199.

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

felixcheung

very cool, thanks

felixcheung · 2017-06-21T17:11:12Z

R/pkg/R/functions.R

-#' @name from_unixtime
-#' @aliases from_unixtime,Column-method
+#' @rdname column_datetime_functions
+#


why a line with # (vs #')?

felixcheung · 2017-06-21T17:12:04Z

R/pkg/R/functions.R

-#' format.
+#' @section Details:
+#' \code{from_unixtime}: Converts the number of seconds from unix epoch (1970-01-01 00:00:00 UTC) to a
+#' string representing the timestamp of that moment in the current system time zone in the given format.


I think we need to call out that "current system time zone" is the one in JVM - in R one could set the default TZ

felixcheung · 2017-06-21T17:14:18Z

R/pkg/R/functions.R

 #' @note to_utc_timestamp since 1.5.0
 setMethod("to_utc_timestamp", signature(y = "Column", x = "character"),
          function(y, x) {
            jc <- callJStatic("org.apache.spark.sql.functions", "to_utc_timestamp", y@jc, x)
            column(jc)
          })

-#' add_months
+#' @section Details:
+#' \code{add_months}: Returns the date that is numMonths after startDate.


might be a bit confusing what is numMonths (x) and what is startDate (y)

Yes, this was the original description. Updated to make it clearer. Also, the examples now will help users figure out how to use these methods.

actually, I wasn't sure this would be the form you are using in other documentation, feel free to massage it, change it.
also might be clearer here to say that is startDate + numMonths - ie. reverse the order the names are mention to be consistent with parameter order

felixcheung · 2017-06-21T17:15:20Z

R/pkg/R/functions.R

-#'
-#' Day of the week parameter is case insensitive, and accepts first three or two characters:
-#' "Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun".
+#' @section Details:


change to @details?

felixcheung · 2017-06-21T17:19:01Z

R/pkg/R/functions.R

+#' @examples
+#'
+#' \dontrun{
+#' set.seed(11)


why need to set seed?

felixcheung · 2017-06-21T17:22:42Z

R/pkg/R/functions.R

+#'
+#' @param y Column to compute on.
+#' @param x For class Column, it is used to perform arithmetic operations with \code{y}.
+#'          For class numeric, it is the number of months or days to be added to \code{y}.


to be added or subtracted?

updated. thx

felixcheung · 2017-06-21T17:25:45Z

R/pkg/R/functions.R

+#'
+#' @param x Column to compute on.
+#' @param format For \code{to_date} and \code{to_timestamp}, it is the string to use to parse
+#'               x Column to DateType or TimestampType. For \code{trunc}, it is the  string used


nit: extra space in the string

felixcheung · 2017-06-21T17:26:14Z

R/pkg/R/functions.R

@@ -546,18 +598,20 @@ setMethod("hash",
            column(jc)
          })

-#' dayofmonth
+#' @section Details:


change to @details

felixcheung · 2017-06-21T17:27:06Z

R/pkg/R/functions.R

 #' @export
-#' @examples \dontrun{date_format(df$t, 'MM/dd/yyy')}


I think this example is not in the new addition

Added back.

felixcheung · 2017-06-21T17:27:44Z

R/pkg/R/functions.R

-#' @examples
-#' \dontrun{
-#' to_timestamp(df$c)
-#' to_timestamp(df$c, 'yyyy-MM-dd')


I think these examples are not added back - could you check?

Added back.

actuaryzhang · 2017-06-21T18:22:31Z

@felixcheung Thanks so much for the review and comments. Super helpful!
I fixed all the issues you have pointed out in the new commit.

SparkQA · 2017-06-21T19:04:09Z

Test build #78403 has finished for PR 18114 at commit a9b1049.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
`#' For class numeric, it is the number of months or days to be added to or subtracted from

SparkQA · 2017-06-21T21:15:44Z

Test build #78407 has finished for PR 18114 at commit 1381dd5.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
`#' @param x For class

actuaryzhang · 2017-06-21T21:36:03Z

@felixcheung Any idea what this message means?
This patch adds the following public classes (experimental): #' @Param x For class

felixcheung

LGTM

felixcheung · 2017-06-22T01:23:18Z

I think it's just the new public class detection thing that

doesn't handle R code at all
is confused by a code line starting with the word class some where

felixcheung · 2017-06-22T03:41:44Z

hmm, waiting for AppVeyor

HyukjinKwon · 2017-06-22T03:50:35Z

@felixcheung, would you give me a moment to double check? I am interested in this and want to help double check.

HyukjinKwon

Sorry, last nitpicking ...

HyukjinKwon · 2017-06-22T03:51:22Z

R/pkg/R/functions.R

+#'
+#' @param y Column to compute on.
+#' @param x For class \code{Column}, it is the column used to perform arithmetic operations
+#'          with column \code{y}.For class \code{numeric}, it is the number of months or


little nit .F ->. F

HyukjinKwon · 2017-06-22T03:52:42Z

R/pkg/R/functions.R

+#'                  to_utc = to_utc_timestamp(df$time, 'PST'),
+#'                  to_unix = unix_timestamp(df$time),
+#'                  to_unix2 = unix_timestamp(df$time, 'yyyy-MM-dd HH'),
+#'                  from_unix = from_unixtime(unix_timestamp(df$time)))


It looks this one should go to column_datetime_functions or from_unixtime should be in column_datetime_diff_functions.

unix_timestamp looks a ditto.

And ... from_unixtime(df$t, 'yyyy/MM/dd HH') looks missed.

~~this I don't get - why from_unixtime belongs to column_datetime_diff_functions?~~
ok I get it. looks like the *timestamp or *time methods here should not in in diff functions group?

Actually, what I found was the documentation for unix_timestamp and from_unixtime was in column_datetime_functions but the examples in column_datetime_diff_functions.

Fixed. The examples for unix_timestamp and from_unixtime are now documented in the correct file.

HyukjinKwon · 2017-06-22T04:01:56Z

R/pkg/R/functions.R

+#'          days to be added to or subtracted from \code{y}. For class \code{character}, it is
+#'          \itemize{
+#'          \item \code{date_format}: date format specification.
+#'          \item \code{from_utc_timestamp, to_utc_timestamp}: time zone to use.


little nit \code{from_utc_timestamp}, \code{to_utc_timestamp}

actuaryzhang · 2017-06-22T05:59:55Z

@HyukjinKwon Great catch. Fixed all issues you pointed out. Thanks!

SparkQA · 2017-06-22T06:50:30Z

Test build #78433 has finished for PR 18114 at commit a291279.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
`#' with column

HyukjinKwon · 2017-06-22T06:55:48Z

This looks good to me too.

felixcheung · 2017-06-22T17:17:00Z

merged to master. thanks!

## What changes were proposed in this pull request? Grouped documentation for datetime column methods. Author: actuaryzhang <actuaryzhang10@gmail.com> Closes apache#18114 from actuaryzhang/sparkRDocDate.

actuaryzhang commented May 25, 2017

View reviewed changes

actuaryzhang changed the title ~~[SPARK-20889][SparkR] Grouped documentation for datetime column methods~~ [SPARK-20889][SparkR] Grouped documentation for DATETIME column methods May 25, 2017

felixcheung reviewed May 26, 2017

View reviewed changes

actuaryzhang force-pushed the sparkRDocDate branch from 0d2853d to 944aa92 Compare May 26, 2017 23:00

actuaryzhang force-pushed the sparkRDocDate branch from 944aa92 to cccab2a Compare May 27, 2017 06:43

actuaryzhang added 3 commits June 19, 2017 20:21

group doc for datetime functions

bdcf726

fix dup arguments

f540a64

update doc for trunc

311ccc2

actuaryzhang force-pushed the sparkRDocDate branch from 016bb47 to 311ccc2 Compare June 20, 2017 03:36

fix dup argument issue

aab9199

felixcheung reviewed Jun 21, 2017

View reviewed changes

address comments

a9b1049

update doc

1381dd5

felixcheung approved these changes Jun 22, 2017

View reviewed changes

HyukjinKwon reviewed Jun 22, 2017

View reviewed changes

correct examples

a291279

felixcheung approved these changes Jun 22, 2017

View reviewed changes

asfgit closed this in 19331b8 Jun 22, 2017

actuaryzhang deleted the sparkRDocDate branch June 22, 2017 17:38

		#' @export
		#' @examples \dontrun{date_format(df$t, 'MM/dd/yyy')}

[SPARK-20889][SparkR] Grouped documentation for DATETIME column methods #18114

[SPARK-20889][SparkR] Grouped documentation for DATETIME column methods #18114

Conversation

actuaryzhang commented May 25, 2017

What changes were proposed in this pull request?

actuaryzhang commented May 25, 2017 • edited

Choose a reason for hiding this comment

actuaryzhang May 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixcheung commented May 26, 2017

actuaryzhang commented May 26, 2017

actuaryzhang commented May 26, 2017

SparkQA commented May 26, 2017

felixcheung commented May 27, 2017

SparkQA commented May 27, 2017

SparkQA commented May 27, 2017

actuaryzhang commented May 27, 2017

SparkQA commented May 27, 2017

actuaryzhang commented Jun 20, 2017

actuaryzhang commented Jun 20, 2017

SparkQA commented Jun 20, 2017

SparkQA commented Jun 20, 2017

felixcheung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixcheung Jun 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

actuaryzhang commented Jun 21, 2017

SparkQA commented Jun 21, 2017

SparkQA commented Jun 21, 2017

actuaryzhang commented Jun 21, 2017

felixcheung left a comment

Choose a reason for hiding this comment

felixcheung commented Jun 22, 2017

felixcheung commented Jun 22, 2017

HyukjinKwon commented Jun 22, 2017

HyukjinKwon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixcheung Jun 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

actuaryzhang commented Jun 22, 2017

SparkQA commented Jun 22, 2017

HyukjinKwon commented Jun 22, 2017

felixcheung commented Jun 22, 2017

actuaryzhang commented May 25, 2017 •

edited

actuaryzhang May 25, 2017 •

edited

felixcheung Jun 21, 2017 •

edited

felixcheung Jun 22, 2017 •

edited