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

join methods for tbl_spark #2651

Merged
merged 2 commits into from Aug 12, 2020
Merged

join methods for tbl_spark #2651

merged 2 commits into from Aug 12, 2020

Conversation

wkdavis
Copy link
Contributor

@wkdavis wkdavis commented Aug 12, 2020

@yl790 It looks like the default test version of Spark is 2.3.0, but I can only run >= 3.0.0 on my machine due to my version of Java, so I will watch the builds to make sure it passes.

Signed-off-by: wkdavis <william.davis@worthingtonindustries.com>
NULL

#' @rdname join.tbl_spark
#' @export
Copy link
Contributor

@yitao-li yitao-li Aug 12, 2020

Choose a reason for hiding this comment

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

Minor: currently the entry for this method is export(inner_join.tbl_spark) in the NAMESPACE file, which is a bit off from the truth.

I think if you make the suggested change below and re-run roxygen2::roxygenize(), the NAMESPACE entry will become S3method(inner_join,tbl_spark), which is more technically correct.

Suggested change
#' @export
#' @importFrom dplyr inner_join
#' @export

And same applies for all other S3 methods in this file.

Copy link
Contributor Author

@wkdavis wkdavis Aug 12, 2020

Choose a reason for hiding this comment

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

Agreed, should be an s3 method. I made the changes - it required reexporting the generics from dplyr in order to register the s3 method (I tried using just the importFrom block as you suggested but that did not register the methods).

Copy link
Contributor

@yitao-li yitao-li left a comment

Looks good overall! Thanks for working on this 👍

My only suggested change is about generating actual S3 method entries in the NAMESPACE file. Let me know what you think.

Signed-off-by: wkdavis <william.davis@worthingtonindustries.com>
@falaki
Copy link
Collaborator

@falaki falaki commented Aug 12, 2020

Databricks Connect tests succeeded. View logs here.

@falaki
Copy link
Collaborator

@falaki falaki commented Aug 12, 2020

Databricks Connect tests succeeded. View logs here.

@yitao-li yitao-li merged commit 255aba2 into sparklyr:master Aug 12, 2020
12 of 13 checks passed
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