Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-8807][SparkR] Add between operator in SparkR #7356

Closed
wants to merge 2 commits into from

Conversation

viirya
Copy link
Member

@viirya viirya commented Jul 11, 2015

JIRA: https://issues.apache.org/jira/browse/SPARK-8807

Add between operator in SparkR.

@SparkQA
Copy link

SparkQA commented Jul 11, 2015

Test build #37091 has finished for PR 7356 at commit c6a25c5.

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

@shivaram
Copy link
Contributor

@sun-rui could you also take a look at this ?

#' @param bounds lower and upper bounds
setMethod("between", signature(x = "Column"),
function(x, bounds) {
if (is.vector(bounds) && length(bounds) == 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we also need to check the types of bound here ? Or to put it another way, does this work for all types or just numbers ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because in Column.scala, the types of lowerBound and upperBound in between function are Any, so I have not checked the types of bound here too.

Copy link
Member Author

Choose a reason for hiding this comment

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

@shivaram Any more comments?

@SparkQA
Copy link

SparkQA commented Jul 16, 2015

Test build #37463 has finished for PR 7356 at commit 7f51b44.

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

@shivaram
Copy link
Contributor

Thanks @viirya -- LGTM.

@asfgit asfgit closed this in 0a79533 Jul 16, 2015
@sun-rui
Copy link
Contributor

sun-rui commented Jul 16, 2015

@viirya,

  1. Why pass the lower bound and upper bound in a vector instead of pass separately? They are passed separately in Scala API. and A vector can not hold complex types (at least a list is expected).
  2. Instead of writing R wrapper functions individually for each method of Scala Column, currently Column.R dynamically creates wrapper function using template. Is it possible to use the same mechanism for supporting between? something like:

column_functions3 <- c("between")

createColumnFunction3 <- function(name) {
setMethod(name,
signature(x = "Column"),
function(x, arg1, arg2) {
... # some check of arg1/arg2
jc <- callJMethod(x@jc, name, arg1, arg2)
column(jc)
})
}

@viirya
Copy link
Member Author

viirya commented Jul 16, 2015

@sun-rui I use the API described in the JIRA ticket. Actually I used two parameters for the lower bound and upper bounds at the first. I am not sure if the between operation supports complex types?

I think it is possibly to have createColumnFunction3 to support it. If there are enough methods with 3 parameters, I think we can create it.

@shivaram
Copy link
Contributor

Hmm I think the scala API having two arguments makes an interesting case -- I did not notice that. What does the Python API do ?

One thing about the createColumnFunction things is that they are hard to document with roxygen. We should create a separate JIRA to discuss this.

@viirya viirya deleted the add_r_between branch December 27, 2023 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants