Skip to content

[SPARK-9871][SparkR] Add expression functions into SparkR which have a variable parameter#8194

Closed
yu-iskw wants to merge 6 commits into
apache:masterfrom
yu-iskw:SPARK-9856
Closed

[SPARK-9871][SparkR] Add expression functions into SparkR which have a variable parameter#8194
yu-iskw wants to merge 6 commits into
apache:masterfrom
yu-iskw:SPARK-9856

Conversation

@yu-iskw

@yu-iskw yu-iskw commented Aug 14, 2015

Copy link
Copy Markdown
Contributor

Summary

  • Add lit function
  • Add concat, greatest, least functions

I think we need to improve collect function in order to implement struct function. Since collect doesn't work with arguments which includes a nested list variable. It seems that a list against struct still has jobj classes. So it would be better to solve this problem on another issue.

JIRA

[SPARK-9871] Add expression functions into SparkR which have a variable parameter - ASF JIRA

@SparkQA

SparkQA commented Aug 14, 2015

Copy link
Copy Markdown

Test build #40858 has finished for PR 8194 at commit 2a5f084.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • implicit class StringToColumn(val sc: StringContext)

@yu-iskw

yu-iskw commented Aug 14, 2015

Copy link
Copy Markdown
Contributor Author

@shivaram could you review it?

Comment thread R/pkg/R/functions.R Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason this is a S3 function unlike the others in this class ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can we define a S4 function which has argument signitures which are like Any? As far as I know, setMethod requires signatures of argument objects.
The argument of lit in Scala is Any. Otherwise, we should define multiple S4 function as follows.

setMethod("lit", signature("numeric"), .....)
setMethod("lit", signature("character"), .....)
...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't know we can use ANY as a signature.

"There are two special classes that can be used in the signature: missing and ANY. missing matches the case where the argument is not supplied, and ANY is used for setting up default methods."
http://adv-r.had.co.nz/S4.html

@shivaram

Copy link
Copy Markdown
Contributor

Thanks @yu-iskw. I left some comments inline. One more thing we need to do is to update our NAMESPACE file [1] with all the functions we have added so far (in this PR and in the previous one).

[1] https://github.com/apache/spark/blob/master/R/pkg/NAMESPACE

@SparkQA

SparkQA commented Aug 15, 2015

Copy link
Copy Markdown

Test build #40938 has finished for PR 8194 at commit 50ed0e5.

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

@SparkQA

SparkQA commented Aug 15, 2015

Copy link
Copy Markdown

Test build #40939 has finished for PR 8194 at commit dc98a13.

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

@SparkQA

SparkQA commented Aug 17, 2015

Copy link
Copy Markdown

Test build #41002 has finished for PR 8194 at commit bf59a84.

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

@yu-iskw

yu-iskw commented Aug 17, 2015

Copy link
Copy Markdown
Contributor Author

@shivaram could you review it?

  • Define lit as a S4 function
  • Add the functions into the NAMESPACE file
  • Remove the unnecessary generic for struct

I have a question about column and col function in SparkR. Should we define them as S4 functions? They are currently defined as S3 functions.

Comment thread R/pkg/NAMESPACE Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lit needs to be added to this list

@shivaram

Copy link
Copy Markdown
Contributor

Thanks @yu-iskw - I had a minor comment about the NAMESPACE but otherwise this LGTM

@yu-iskw

yu-iskw commented Aug 17, 2015

Copy link
Copy Markdown
Contributor Author

@shivaram thank you for your comment. I updated it!

@SparkQA

SparkQA commented Aug 17, 2015

Copy link
Copy Markdown

Test build #41006 has finished for PR 8194 at commit 36407ef.

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

@shivaram

Copy link
Copy Markdown
Contributor

LGTM Merging this

asfgit pushed a commit that referenced this pull request Aug 17, 2015
… a variable parameter

### Summary

- Add `lit` function
- Add `concat`, `greatest`, `least` functions

I think we need to improve `collect` function in order to implement `struct` function. Since `collect` doesn't work with arguments which includes a nested `list` variable. It seems that a list against `struct` still has `jobj` classes. So it would be better to solve this problem on another issue.

### JIRA
[[SPARK-9871] Add expression functions into SparkR which have a variable parameter - ASF JIRA](https://issues.apache.org/jira/browse/SPARK-9871)

Author: Yu ISHIKAWA <yuu.ishikawa@gmail.com>

Closes #8194 from yu-iskw/SPARK-9856.

(cherry picked from commit 26e7605)
Signed-off-by: Shivaram Venkataraman <shivaram@cs.berkeley.edu>
@asfgit asfgit closed this in 26e7605 Aug 17, 2015
@yu-iskw

yu-iskw commented Aug 17, 2015

Copy link
Copy Markdown
Contributor Author

Thank you for merging it!

CodingCat pushed a commit to CodingCat/spark that referenced this pull request Aug 17, 2015
… a variable parameter

### Summary

- Add `lit` function
- Add `concat`, `greatest`, `least` functions

I think we need to improve `collect` function in order to implement `struct` function. Since `collect` doesn't work with arguments which includes a nested `list` variable. It seems that a list against `struct` still has `jobj` classes. So it would be better to solve this problem on another issue.

### JIRA
[[SPARK-9871] Add expression functions into SparkR which have a variable parameter - ASF JIRA](https://issues.apache.org/jira/browse/SPARK-9871)

Author: Yu ISHIKAWA <yuu.ishikawa@gmail.com>

Closes apache#8194 from yu-iskw/SPARK-9856.
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.

3 participants