[SPARK-10807][SPARKR] Added as.data.frame as a synonym for collect #8908

olarayej · 2015-09-24T21:06:43Z

Created method as.data.frame as a synonym for collect().

shivaram · 2015-09-24T21:19:46Z

Jenkins, ok to test

SparkQA · 2015-09-24T21:30:35Z

Test build #42981 has finished for PR 8908 at commit e9e34b5.

This patch fails R style tests.
This patch merges cleanly.
This patch adds no public classes.

olarayej · 2015-09-24T22:22:08Z

How can I check the style in my local environment? I ran the unit tests but got no issues. Tried the link below, but it's only for Python and Scala: https://cwiki.apache.org/confluence/display/SPARK/Spark+Code+Style+Guide,

shivaram · 2015-09-24T22:26:37Z

You can run dev/lint-r in your tree and it should do the style checks. Also the jenkins log has the error at the end

R/DataFrame.R:1867:69: style: Trailing whitespace is superfluous.
setMethod(f = "as.data.frame", signature = "DataFrame", definition =

olarayej · 2015-09-24T22:59:25Z

Yeah, I saw that one. Thank you so much! I have fixed it, and ran dev/lint-r on my local. It should be good now :)

SparkQA · 2015-09-24T23:22:25Z

Test build #42989 has finished for PR 8908 at commit cee871c.

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

shivaram · 2015-09-25T17:55:24Z

R/pkg/R/DataFrame.R

Minor style comments -- To keep this similar to the other methods in this function I think this can be

setMethod("as.data.frame", signature(x = "DataFrame"), function(x, ...) { }

shivaram · 2015-09-25T17:58:03Z

Thanks @olarayej -- I just had a couple of minor style comments

One more thing is that it will be good to add a unit test for this. You could just add a test case to some of the existing tests at

spark/R/pkg/inst/tests/test_sparkSQL.R

Line 215 in 9223388

test_that("create DataFrame from list or data.frame", {

shivaram · 2015-09-25T17:59:37Z

R/pkg/R/DataFrame.R

We have all the generics used in SparkR in R/generics.R -- Could you move this to that file as well ?

shivaram · 2015-09-25T18:41:52Z

Also could you change the PR title to [SPARK-10807][SPARKR] ? This matches the format we use for all PRs -- https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark#ContributingtoSpark-PullRequest has more information

Changed setMethod stle Removed return()

shivaram · 2015-09-28T18:29:23Z

@olarayej Sorry I wasn't clear the last time around but the title should be [SPARK-10807][SPARKR] Added as.data.frame as a synonym for collect

Conflicts: R/pkg/R/DataFrame.R

olarayej · 2015-09-28T19:24:46Z

@shivaram Thanks a lot for your comments! I have addressed them.
-Oscar

shivaram · 2015-09-28T20:05:53Z

R/pkg/R/DataFrame.R

sorry a couple of more style nits. Based on existing code, this should be

setMethod("as.data.frame", signature(x = "DataFrame"),

(i.e. having x = in the signature line and no f= in the name line)
Again, just to restate this isn't for functionality but just for matching the style with all the existing code

Sorry I missed that! I have fixed it now.

SparkQA · 2015-09-28T20:58:05Z

Test build #43072 has finished for PR 8908 at commit a346cc6.

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

shivaram · 2015-09-28T21:04:19Z

Jenkins, retest this please

SparkQA · 2015-09-28T21:45:03Z

Test build #43069 has finished for PR 8908 at commit de6d164.

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

shivaram · 2015-09-28T21:56:42Z

Jenkins, retest this please

felixcheung · 2015-09-29T00:24:18Z

R/pkg/NAMESPACE

is there a reason why this isn't on the same list of export for DataFrame above?

SparkQA · 2015-09-29T16:21:51Z

Test build #43086 has finished for PR 8908 at commit a346cc6.

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

shivaram · 2015-09-29T17:48:36Z

Jenkins, retest this please

SparkQA · 2015-09-29T20:07:08Z

Test build #43097 has finished for PR 8908 at commit a346cc6.

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

shivaram · 2015-09-29T20:11:07Z

@shaneknapp The log has a strange error I haven't seen so far

/home/jenkins/workspace/SparkPullRequestBuilder/build/sbt -Pyarn -Phadoop-2.3 -Dhadoop.version=2.3.0 -Phive -Pkinesis-asl -Phive-thriftserver test ; received return code 143

Any ideas if this is related to the recent Jenkins issues ?

shaneknapp · 2015-09-29T20:58:44Z

huh, i haven't seen that error message either. when i run that command locally (same box, same working dir, jenkins user), it works. i'll try and keep an eye out for failures like this, but please let me know if you see something like this again.

olarayej · 2015-09-30T00:28:41Z

@shivaram @shaneknapp: any suggestions? I did double check the unit tests work fine on my environment. Thanks!

shivaram · 2015-09-30T00:29:58Z

Jenkins, retest this please

SparkQA · 2015-09-30T02:56:51Z

Test build #43106 has finished for PR 8908 at commit a346cc6.

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

shivaram · 2015-09-30T03:40:39Z

@shaneknapp Looks like the same error again.

@olarayej Could you remove the change to .gitignore in this PR ? Because of the way our tests are setup only changes in R/ will only run SparkR unit tests which should pass without a problem I think

shaneknapp · 2015-09-30T04:29:09Z

i'll take a closer look tomorrow, @shivaram.

On Tue, Sep 29, 2015 at 8:41 PM, Shivaram Venkataraman <
[email protected]> wrote:

@shaneknapp https://github.com/shaneknapp Looks like the same error
again.

@olarayej https://github.com/olarayej Could you remove the change to
.gitignore in this PR ? Because of the way our tests are setup only
changes in R/ will only run SparkR unit tests which should pass without a
problem I think

—
Reply to this email directly or view it on GitHub
#8908 (comment).

olarayej · 2015-09-30T18:14:06Z

@shivaram I have removed the changes to .gitignore. My apologies, I accidentally committed this file. Thanks!

SparkQA · 2015-09-30T18:42:29Z

Test build #43136 has finished for PR 8908 at commit 6c4dcbc.

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

olarayej · 2015-09-30T18:45:34Z

Awesome! :-)

shivaram · 2015-09-30T21:11:51Z

Thanks @olarayej LGTM. Merging this

Oscar D. Lara Yejas and others added 2 commits September 24, 2015 22:01

SPARK-10807. Added as.data.frame as a synonym for collect().

461714d

Removed operator %++%, which is a synonym for paste()

e9e34b5

Oscar D. Lara Yejas and others added 2 commits September 24, 2015 23:53

Removed extra blank space.

c65b682

Removed extra spaces to comply with R style

cee871c

shivaram reviewed Sep 25, 2015
View reviewed changes

olarayej changed the title ~~SPARK-10807. Added as.data.frame as a synonym for collect().~~ [SPARK-10807][SPARKR] Sep 28, 2015

Moved setGeneric declaration to generics.R.

0851163

Changed setMethod stle Removed return()

olarayej changed the title ~~[SPARK-10807][SPARKR]~~ [SPARK-10807][SPARKR] Added as.data.frame as a synonym for collect Sep 28, 2015

Oscar D. Lara Yejas added 2 commits September 28, 2015 19:48

Added test cases for as.data.frame

7a8e62a

Merge remote-tracking branch 'origin/SPARK-10807' into SPARK-10807

de6d164

Conflicts: R/pkg/R/DataFrame.R

shivaram reviewed Sep 28, 2015
View reviewed changes

Changed setMethod declaration to comply with standard

a346cc6

felixcheung reviewed Sep 29, 2015
View reviewed changes

R/pkg/NAMESPACE

Copy link

Member

felixcheung Sep 29, 2015

Choose a reason for hiding this comment

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

is there a reason why this isn't on the same list of export for DataFrame above?

Removed changes to .gitignore

6c4dcbc

asfgit closed this in f21e2da Oct 1, 2015

[SPARK-10807][SPARKR] Added as.data.frame as a synonym for collect #8908

[SPARK-10807][SPARKR] Added as.data.frame as a synonym for collect #8908

Uh oh!

Conversation

olarayej commented Sep 24, 2015

Uh oh!

shivaram commented Sep 24, 2015

Uh oh!

SparkQA commented Sep 24, 2015

Uh oh!

olarayej commented Sep 24, 2015

Uh oh!

shivaram commented Sep 24, 2015

Uh oh!

olarayej commented Sep 24, 2015

Uh oh!

SparkQA commented Sep 24, 2015

Uh oh!

shivaram Sep 25, 2015

Choose a reason for hiding this comment

Uh oh!

shivaram commented Sep 25, 2015

Uh oh!

shivaram Sep 25, 2015

Choose a reason for hiding this comment

Uh oh!

shivaram commented Sep 25, 2015

Uh oh!

shivaram commented Sep 28, 2015

Uh oh!

olarayej commented Sep 28, 2015

Uh oh!

shivaram Sep 28, 2015

Choose a reason for hiding this comment

Uh oh!

olarayej Sep 28, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 28, 2015

Uh oh!

shivaram commented Sep 28, 2015

Uh oh!

SparkQA commented Sep 28, 2015

Uh oh!

shivaram commented Sep 28, 2015

Uh oh!

felixcheung Sep 29, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 29, 2015

Uh oh!

shivaram commented Sep 29, 2015

Uh oh!

SparkQA commented Sep 29, 2015

Uh oh!

shivaram commented Sep 29, 2015

Uh oh!

shaneknapp commented Sep 29, 2015

Uh oh!

olarayej commented Sep 30, 2015

Uh oh!

shivaram commented Sep 30, 2015

Uh oh!

SparkQA commented Sep 30, 2015

Uh oh!

shivaram commented Sep 30, 2015

Uh oh!

shaneknapp commented Sep 30, 2015

Uh oh!

olarayej commented Sep 30, 2015

Uh oh!

SparkQA commented Sep 30, 2015

Uh oh!

olarayej commented Sep 30, 2015

Uh oh!

shivaram commented Sep 30, 2015

Uh oh!

Reviewers

Assignees

Labels