Skip to content

[SPARK-25117][R] Add EXEPT ALL and INTERSECT ALL support in R#22107

Closed
dilipbiswal wants to merge 6 commits into
apache:masterfrom
dilipbiswal:SPARK-25117
Closed

[SPARK-25117][R] Add EXEPT ALL and INTERSECT ALL support in R#22107
dilipbiswal wants to merge 6 commits into
apache:masterfrom
dilipbiswal:SPARK-25117

Conversation

@dilipbiswal

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

SPARK-21274 added support for EXCEPT ALL and INTERSECT ALL. This PR adds the support in R.

How was this patch tested?

Added test in test_sparkSQL.R

@SparkQA

SparkQA commented Aug 14, 2018

Copy link
Copy Markdown

Test build #94760 has finished for PR 22107 at commit 426ffed.

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

@gatorsmile

Copy link
Copy Markdown
Member

cc @felixcheung

@SparkQA

SparkQA commented Aug 14, 2018

Copy link
Copy Markdown

Test build #94764 has finished for PR 22107 at commit 7e88c9d.

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

@dilipbiswal dilipbiswal changed the title [SPARK-25117] Add EXEPT ALL and INTERSECT ALL support in R [SPARK-25117][R] Add EXEPT ALL and INTERSECT ALL support in R Aug 14, 2018
Comment thread R/pkg/R/DataFrame.R Outdated
#' intersectAllDF <- intersectAll(df1, df2)
#' }
#' @rdname intersectAll
#' @note intersectAll since 2.4

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

please put 2.4.0

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.

@SparkQA

SparkQA commented Aug 15, 2018

Copy link
Copy Markdown

Test build #94777 has finished for PR 22107 at commit 5247ab5.

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

Comment thread R/pkg/tests/fulltests/test_sparkSQL.R Outdated
list("a", 1),
list("a", 1),
list("b", 3),
list("c", 4)),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit:

list(list("a", 1), list("a", 1), list("a", 1),
     list("a", 1), list("b", 3), list("c", 4)),

Comment thread R/pkg/tests/fulltests/test_sparkSQL.R Outdated
schema = c("a", "b"))
df2 <- createDataFrame(
list(list("a", 1), list("a", 1), list("b", 3)),
schema = c("a", "b"))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit:

df2 <- createDataFrame(list(list("a", 1), list("a", 1), list("b", 3)), schema = c("a", "b"))

Comment thread R/pkg/tests/fulltests/test_sparkSQL.R Outdated
stringsAsFactors = FALSE)
except_all_expected <- data.frame("a" = c("a", "a", "c"), "b" = c(1, 1, 4),
stringsAsFactors = FALSE)
intersect_all_df <- arrange(intersectAll(df1, df2), df1$a)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Strictly, the naming rule is intersectAllDf or intersect.all.df (see #17590 (comment))

@HyukjinKwon

Copy link
Copy Markdown
Member

Seems fine.

@SparkQA

SparkQA commented Aug 15, 2018

Copy link
Copy Markdown

Test build #94789 has finished for PR 22107 at commit 1d93304.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon

Copy link
Copy Markdown
Member

retest this please

@SparkQA

SparkQA commented Aug 15, 2018

Copy link
Copy Markdown

Test build #94791 has finished for PR 22107 at commit 1d93304.

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

Comment thread R/pkg/R/DataFrame.R Outdated
#' df2 <- read.json(path2)
#' exceptAllDF <- exceptAll(df1, df2)
#' }
#' @rdname exceptAll

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is a bug in except there should only be one @rdname for each

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.

@felixcheung Thanks .. Did you want the original function except fixed at part of this ?

Comment thread R/pkg/R/DataFrame.R Outdated
#' df2 <- read.json(path2)
#' intersectAllDF <- intersectAll(df1, df2)
#' }
#' @rdname intersectAll

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ditto here

Comment thread R/pkg/R/DataFrame.R
function(x, y) {
intersected <- callJMethod(x@sdf, "intersectAll", y@sdf)
dataFrame(intersected)
})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

add extra empty line after code

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.

Comment thread R/pkg/R/DataFrame.R
excepted <- callJMethod(x@sdf, "exceptAll", y@sdf)
dataFrame(excepted)
})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: remove one of the two empty lines

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.

@felixcheung Sure.

@SparkQA

SparkQA commented Aug 16, 2018

Copy link
Copy Markdown

Test build #94844 has finished for PR 22107 at commit 528050d.

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

@dilipbiswal

Copy link
Copy Markdown
Contributor Author

@felixcheung I have incorporated the comments.

@felixcheung felixcheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@HyukjinKwon HyukjinKwon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM too

@asfgit asfgit closed this in 162326c Aug 17, 2018
@felixcheung

Copy link
Copy Markdown
Member

merged to master

@dilipbiswal

Copy link
Copy Markdown
Contributor Author

Thank you very much @HyukjinKwon @felixcheung

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.

6 participants