Skip to content

Conversation

@sun-rui
Copy link
Contributor

@sun-rui sun-rui commented Oct 21, 2015

No description provided.

@SparkQA
Copy link

SparkQA commented Oct 21, 2015

Test build #44043 has finished for PR 9193 at commit ed57eb8.

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

Copy link
Member

Choose a reason for hiding this comment

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

nit: this is kinda hard to read with the double negatives. consider:

if ((parameterType.isPrimitive || args(i) != null) &&
    !parameterWrapperType.isInstance(args(i))) {

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. done.

Anyway, I feel painful that Scala does not support 'continue' statement. So the code here still not so good.

@SparkQA
Copy link

SparkQA commented Oct 21, 2015

Test build #44050 has finished for PR 9193 at commit 0504092.

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

@SparkQA
Copy link

SparkQA commented Oct 21, 2015

Test build #44051 has finished for PR 9193 at commit 50a0d27.

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

@sun-rui
Copy link
Contributor Author

sun-rui commented Oct 22, 2015

Jenkins, retest this please

@SparkQA
Copy link

SparkQA commented Oct 22, 2015

Test build #44128 has finished for PR 9193 at commit 50a0d27.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a method called lag in base R that this would conflict with. Could we try to use the same argument names as that function ?

@sun-rui
Copy link
Contributor Author

sun-rui commented Oct 26, 2015

@shivaram, R lag function is for time series objects, which is irrelevant to the lead/lag here in SQL window functions.

There are some functions in dplyr package mimicing SQL window functions, including lead and lag. Refer to https://cran.r-project.org/web/packages/dplyr/vignettes/window-functions.html and https://cran.r-project.org/web/packages/dplyr/dplyr.pdf. But a quick glance at these functions shows that they are working on vectors (for example, lag right shifts a vector by offset), so the semantic of them are different from those in Spark.

So for now I won't change this PR.

@shivaram
Copy link
Contributor

Ok. The thing I was worried about is that if somebody loads SparkR they won't be able to use lag from stats -- Can you check if that is the case ?

@sun-rui
Copy link
Contributor Author

sun-rui commented Oct 27, 2015

Yes. the lag will be masked. Just as discussed before, sometimes, this is allowed, as I assume lag is not so commonly and frequently used ("dplyr" masks lag also). Users can still use stats::lag if they do want to use it.

@shivaram
Copy link
Contributor

Hmm I see. In the case of lag I can see your point. But at a high level I think we should a make a list of functions that we mask and are incompatible (i.e. things like head are not a problem)

Otherwise LGTM. Merging this.

@asfgit asfgit closed this in dc3220c Oct 27, 2015
@sun-rui
Copy link
Contributor Author

sun-rui commented Oct 27, 2015

@shivaram, for your suggestion that "we should a make a list of functions that we mask and are incompatible", I submitted https://issues.apache.org/jira/browse/SPARK-11339

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.

4 participants