[SPARK-20371][R] Add wrappers for collect_list and collect_set #17672

zero323 · 2017-04-18T17:52:02Z

What changes were proposed in this pull request?

Adds wrappers for collect_list and collect_set.

How was this patch tested?

Unit tests, check-cran.sh

SparkQA · 2017-04-18T18:26:48Z

Test build #75906 has finished for PR 17672 at commit 3b580ae.

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

SparkQA · 2017-04-18T19:05:37Z

Test build #75909 has finished for PR 17672 at commit e5bcb67.

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

SparkQA · 2017-04-18T20:55:04Z

Test build #75911 has finished for PR 17672 at commit a02a119.

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

zero323 · 2017-04-18T21:14:26Z

cc @felixcheung

felixcheung · 2017-04-19T02:33:01Z

R/pkg/R/functions.R

+
+#' collect_list
+#'
+#' Aggregate function: returns a list of objects with duplicates.


does other function has Aggregate function: in the description or this is carried over from Scala doc?
if latter we could go without - there's already a @family tag

From Scala docs. Removed.

felixcheung · 2017-04-19T02:34:59Z

R/pkg/R/functions.R

+#'
+#' @rdname collect_list
+#' @name collect_list
+#' @family aggregate_functions


I think there's an existing agg_funcs in R

Yes, corrected.

re: my other comments, my preference would be full text without underscore like @family aggregate functions. This isn't a key or id and it shows up in generated doc text. Anyway, that's a bigger change all around.

felixcheung · 2017-04-19T02:36:39Z

R/pkg/R/generics.R

@@ -918,6 +918,14 @@ setGeneric("cbrt", function(x) { standardGeneric("cbrt") })
 #' @export
 setGeneric("ceil", function(x) { standardGeneric("ceil") })

+#' @rdname collect_list


these are under ###################### Expression Function Methods ##########################
which doesn't seem like the right group

Let's continue here #17674 (comment)

ok, it's good.

felixcheung · 2017-04-19T02:37:03Z

R/pkg/inst/tests/testthat/test_sparkSQL.R

+    agg(gd3, collect_set(df8$age), collect_list(df8$age))
+  )
+
+  testthat::expect_equal(


why testthat::?

Just a habit. Corrected.

SparkQA · 2017-04-19T11:42:57Z

Test build #75940 has finished for PR 17672 at commit fdc0395.

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

felixcheung · 2017-04-20T04:11:59Z

R/pkg/R/functions.R

+#'
+#' @rdname collect_list
+#' @name collect_list
+#' @family agg_func


it has an 's' though agg_funcs - not to nit pick but it needs to match exactly

My bad.

On a side note, do you think we should provide detailed examples for each function?

we really should - it's a long standing concern that something like

@examples \dontrun{collect_list(df$x)}

is not useful at all. At least we should include how this goes into a select statement, and in some cases what the output looks like. However, with the number of sql functions we have it's a bit of ongoing work to improve this and we definitely can use more help on that

SparkQA · 2017-04-20T11:05:48Z

Test build #75982 has finished for PR 17672 at commit a089495.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-04-20T14:36:31Z

Test build #75988 has finished for PR 17672 at commit 944f5b4.

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

felixcheung · 2017-04-20T16:57:47Z

btw, fyi, you don't have to rebase or squash each time you push - it's actually easier to review if you don't - so reviewer can track comments and see the diff from the last time.

but I get that you might be rebasing because of conflict.

felixcheung

LGTM

zero323 · 2017-04-20T19:10:45Z

btw, fyi, you don't have to rebase or squash each time you push

Noted :) But yeah, there was a conflict after SPARK-20375.

At least we should include how this goes into a select statement, and in some cases what the output looks like.

It would be great.

felixcheung · 2017-04-21T19:06:34Z

merged to master.

zero323 · 2017-04-21T19:46:05Z

Thanks @felixcheung

zero323 · 2017-04-22T00:27:29Z

BTW @felixcheung - is there any deeper reason behind current stat of generics.R? I mean:

Inconsistent usage of standard and roxygen comments.
Marking functions which are not to be exported with @export.
Slightly mixed up order (both in groups and between groups).
Some minor inconsistencies (like marking asc as @rdname columnfunctions).

felixcheung · 2017-04-22T00:32:59Z

Not really its just inconsistent handling. Some comment changes can be deliberated though.

zero323 · 2017-04-22T00:56:07Z

Yeah, I have this feeling that it could be deliberate, but I cannot figure out what is the purpose. Removing @exports should be enough, shouldn't it?

I thought about cleaning this up, but I wonder if it is better to wait for SPARK-16693.

felixcheung · 2017-04-22T02:44:22Z

Will be probably cleaner

zero323 · 2017-05-15T11:00:51Z

@felixcheung Do you know by any chance what is the policy about adding new datasets to Spark? License restrictions, file size and such?

felixcheung · 2017-05-16T04:13:41Z

@zero323 I think that its license needs to be compatible with Apache 2.0 and it can't be big (since example data is in the release; no more than a few MB?) https://www.apache.org/licenses/

## What changes were proposed in this pull request? Adds wrappers for `collect_list` and `collect_set`. ## How was this patch tested? Unit tests, `check-cran.sh` Author: zero323 <zero323@users.noreply.github.com> Closes apache#17672 from zero323/SPARK-20371.

zero323 force-pushed the SPARK-20371 branch from 3b580ae to e5bcb67 Compare April 18, 2017 18:30

zero323 force-pushed the SPARK-20371 branch from e5bcb67 to a02a119 Compare April 18, 2017 20:18

felixcheung requested changes Apr 19, 2017

View reviewed changes

zero323 force-pushed the SPARK-20371 branch from a02a119 to fdc0395 Compare April 19, 2017 11:06

felixcheung requested changes Apr 20, 2017

View reviewed changes

zero323 force-pushed the SPARK-20371 branch from fdc0395 to a089495 Compare April 20, 2017 10:29

Add wrappers for collect_list and collect_set

944f5b4

zero323 force-pushed the SPARK-20371 branch from a089495 to 944f5b4 Compare April 20, 2017 13:58

felixcheung approved these changes Apr 20, 2017

View reviewed changes

asfgit closed this in fd648bf Apr 21, 2017

zero323 deleted the SPARK-20371 branch April 26, 2017 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-20371][R] Add wrappers for collect_list and collect_set #17672

[SPARK-20371][R] Add wrappers for collect_list and collect_set #17672

zero323 commented Apr 18, 2017

SparkQA commented Apr 18, 2017

SparkQA commented Apr 18, 2017

SparkQA commented Apr 18, 2017

zero323 commented Apr 18, 2017

felixcheung Apr 19, 2017

zero323 Apr 19, 2017

felixcheung Apr 19, 2017

zero323 Apr 19, 2017

felixcheung Apr 19, 2017

felixcheung Apr 19, 2017

zero323 Apr 19, 2017

felixcheung Apr 19, 2017

felixcheung Apr 19, 2017

zero323 Apr 19, 2017

SparkQA commented Apr 19, 2017

felixcheung Apr 20, 2017

zero323 Apr 20, 2017

felixcheung Apr 20, 2017

SparkQA commented Apr 20, 2017

SparkQA commented Apr 20, 2017

felixcheung commented Apr 20, 2017 •

edited

felixcheung left a comment

zero323 commented Apr 20, 2017

felixcheung commented Apr 21, 2017

zero323 commented Apr 21, 2017

zero323 commented Apr 22, 2017

felixcheung commented Apr 22, 2017 via email

zero323 commented Apr 22, 2017

felixcheung commented Apr 22, 2017 via email

zero323 commented May 15, 2017

felixcheung commented May 16, 2017

[SPARK-20371][R] Add wrappers for collect_list and collect_set #17672

[SPARK-20371][R] Add wrappers for collect_list and collect_set #17672

Conversation

zero323 commented Apr 18, 2017

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Apr 18, 2017

SparkQA commented Apr 18, 2017

SparkQA commented Apr 18, 2017

zero323 commented Apr 18, 2017

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

SparkQA commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Apr 20, 2017

SparkQA commented Apr 20, 2017

felixcheung commented Apr 20, 2017 • edited

felixcheung left a comment

Choose a reason for hiding this comment

zero323 commented Apr 20, 2017

felixcheung commented Apr 21, 2017

zero323 commented Apr 21, 2017

zero323 commented Apr 22, 2017

felixcheung commented Apr 22, 2017 via email

zero323 commented Apr 22, 2017

felixcheung commented Apr 22, 2017 via email

zero323 commented May 15, 2017

felixcheung commented May 16, 2017

felixcheung commented Apr 20, 2017 •

edited