[SPARK-16012][SparkR] Implement gapplyCollect which will apply a R function on each group similar to gapply and collect the result back to R data.frame #13760

NarineK · 2016-06-18T09:06:32Z

What changes were proposed in this pull request?

gapplyCollect() does gapply() on a SparkDataFrame and collect the result back to R. Compared to gapply() + collect(), gapplyCollect() offers performance optimization as well as programming convenience, as no schema is needed to be provided.

This is similar to dapplyCollect().

How was this patch tested?

Added test cases for gapplyCollect similar to dapplyCollect

SparkQA · 2016-06-18T09:07:43Z

Test build #60777 has finished for PR 13760 at commit de5dbb0.

This patch fails some tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-06-18T09:17:42Z

Test build #60778 has finished for PR 13760 at commit 4b9cd3e.

This patch fails some tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-06-18T09:53:17Z

Test build #60779 has finished for PR 13760 at commit f96fae9.

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

SparkQA · 2016-06-19T18:56:57Z

Test build #60815 has finished for PR 13760 at commit 7dd883f.

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

SparkQA · 2016-06-19T21:09:17Z

Test build #60818 has finished for PR 13760 at commit 11c7cd6.

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

shivaram · 2016-06-19T21:22:16Z

Thanks @NarineK -- cc @sun-rui for review

…or gapply and gapplyCollect

felixcheung · 2016-06-20T22:21:34Z

R/pkg/R/group.R

-#' @return a SparkDataFrame
 #' @rdname gapply
 #' @name gapply
+#' @seealso gapplyCollect \link{gapplyCollect}


you can leave it as #' @seealso \link{gapplyCollect}

please add @export

SparkQA · 2016-06-20T23:25:24Z

Test build #60871 has finished for PR 13760 at commit 21de08f.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

sun-rui · 2016-06-21T02:33:35Z

R/pkg/R/DataFrame.R

+#'
+#' result <- gapplyCollect(
+#'   df,
+#'   list("a", "c"),


use c("a", "c") is more natural?

SparkQA · 2016-06-21T23:06:23Z

Test build #60971 has finished for PR 13760 at commit 1d62c38.

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

SparkQA · 2016-06-21T23:07:39Z

Test build #60972 has finished for PR 13760 at commit 77fb205.

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

sun-rui · 2016-06-22T01:51:10Z

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

  actual <- collect(df1)
  expect_identical(actual, expected)

+  df1Collect <- gapplyCollect(df, list("a"), function(key, x) { x })


maybe better change list("a") to "a" to test if a scalar column parameter can work

we should have both?

sun-rui · 2016-06-22T01:51:32Z

LGTM except on minor comment

shivaram · 2016-06-22T16:32:09Z

R/pkg/R/group.R

 setMethod("gapply",
          signature(x = "GroupedData"),
          function(x, func, schema) {
-            try(if (is.null(schema)) stop("schema cannot be NULL"))


this check of schema not being null still needs to be preserved for the the gapply call ?

yeah, we need it. I tried to do it like dapply but dapply forces it by signature and gapply not.
will bring it back thnx

SparkQA · 2016-06-22T20:24:09Z

Test build #61064 has finished for PR 13760 at commit 022f87d.

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

felixcheung · 2016-06-23T16:44:02Z

R/pkg/R/group.R

 setMethod("gapply",
          signature(x = "GroupedData"),
          function(x, func, schema) {
            try(if (is.null(schema)) stop("schema cannot be NULL"))


why do have it inside try again? don't we want this to fail?

with or without try both work work fine.

Without try the error looks like:
Error in .local(x, ...) : schema cannot be NULL

with try:
Error in try(if (is.null(schema)) stop("schema cannot be NULL")) :
schema cannot be NULL

Is there a convention in SparkR for showing an error message ?

I think we use stop or warn directly in SparkR without wrapping in a try block. I think the try block is useful if we want to catch an error of one kind and then reformat it or show a different error etc. In this case stop should be sufficient.

SparkQA · 2016-06-23T21:04:42Z

Test build #61125 has finished for PR 13760 at commit 963f14f.

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

shivaram · 2016-06-25T01:57:51Z

@felixcheung Any other comments on this ?

felixcheung · 2016-06-27T08:43:03Z

R/pkg/R/DataFrame.R

 #' schema <-  structType(structField("a", "integer"), structField("c", "string"),
 #'   structField("avg", "double"))
-#' df1 <- gapply(
+#' result <- gapply(


if what is returned is a DataFrame it might help to keep it a variant of "df"
in fact, you might want to add @return to document to return value and type

thanks, @felixcheung , I think I kept it consistent with dapply/dapplyCollect. Those do not have @return. I can add it to gapply

SparkQA · 2016-06-27T13:27:38Z

Test build #61300 has finished for PR 13760 at commit 1e3f0ac.

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

NarineK · 2016-06-28T15:56:52Z

@felixcheung , I've addressed the comments or put a comment for the non-addressed ones.

felixcheung · 2016-06-29T07:55:43Z

looks good, thanks!

NarineK · 2016-06-30T14:25:40Z

Do you have any questions on this @shivaram , @sun-rui ?

sun-rui · 2016-07-01T02:17:59Z

no

shivaram · 2016-07-01T20:54:35Z

Thanks all. LGTM. Merging this to master and branch-2.0

…nction on each group similar to gapply and collect the result back to R data.frame ## What changes were proposed in this pull request? gapplyCollect() does gapply() on a SparkDataFrame and collect the result back to R. Compared to gapply() + collect(), gapplyCollect() offers performance optimization as well as programming convenience, as no schema is needed to be provided. This is similar to dapplyCollect(). ## How was this patch tested? Added test cases for gapplyCollect similar to dapplyCollect Author: Narine Kokhlikyan <[email protected]> Closes #13760 from NarineK/gapplyCollect. (cherry picked from commit 26afb4c) Signed-off-by: Shivaram Venkataraman <[email protected]>

Narine Kokhlikyan added 5 commits June 17, 2016 11:51

initial version of gapplyCollect

ea31820

merged with master

f8e54dc

revert .gitignore

591c480

added test cases + improved the code

37b633a

fixed test case

de5dbb0

correct @name documentation tag

4b9cd3e

fixed @name tag in gapplyCollect

f96fae9

remove schema about schema

7dd883f

fix gapplyCollect test cases

11c7cd6

remove dupplicated comments from documentation + separate @rdname-s f…

21de08f

…or gapply and gapplyCollect

felixcheung reviewed Jun 20, 2016
View reviewed changes

sun-rui reviewed Jun 21, 2016
View reviewed changes

R/pkg/R/DataFrame.R Outdated

#'

#' result <- gapplyCollect(

#' df,

#' list("a", "c"),

Copy link

Contributor

sun-rui Jun 21, 2016

Choose a reason for hiding this comment

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

use c("a", "c") is more natural?

Narine Kokhlikyan added 2 commits June 22, 2016 01:26

merge with master + addessed sun-rui's and felix's comments

1d62c38

fix exports

77fb205

sun-rui reviewed Jun 22, 2016
View reviewed changes

shivaram reviewed Jun 22, 2016
View reviewed changes

addressed Shivaram's and sun-rui's comments

022f87d

felixcheung reviewed Jun 23, 2016
View reviewed changes

move comments from group.R to DataFrame.R

963f14f

felixcheung reviewed Jun 27, 2016
View reviewed changes

Addressed Felix's comments

1e3f0ac

asfgit closed this in 26afb4c Jul 1, 2016

[SPARK-16012][SparkR] Implement gapplyCollect which will apply a R function on each group similar to gapply and collect the result back to R data.frame #13760

[SPARK-16012][SparkR] Implement gapplyCollect which will apply a R function on each group similar to gapply and collect the result back to R data.frame #13760

Uh oh!

Conversation

NarineK commented Jun 18, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jun 18, 2016

Uh oh!

SparkQA commented Jun 18, 2016

Uh oh!

SparkQA commented Jun 18, 2016

Uh oh!

SparkQA commented Jun 19, 2016

Uh oh!

SparkQA commented Jun 19, 2016

Uh oh!

shivaram commented Jun 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 21, 2016

Uh oh!

SparkQA commented Jun 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sun-rui commented Jun 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 23, 2016

Uh oh!

shivaram commented Jun 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 27, 2016

Uh oh!

NarineK commented Jun 28, 2016

Uh oh!

felixcheung commented Jun 29, 2016

Uh oh!

NarineK commented Jun 30, 2016

Uh oh!

sun-rui commented Jul 1, 2016

Uh oh!

shivaram commented Jul 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants