[SPARK-20490][SPARKR] Add R wrappers for eqNullSafe and ! / not #17783

zero323 · 2017-04-27T14:13:41Z

What changes were proposed in this pull request?

Add null-safe equality operator %<=>% (sames as o.a.s.sql.Column.eqNullSafe, o.a.s.sql.Column.<=>)
Add boolean negation operator ! and function not .

How was this patch tested?

Existing unit tests, additional unit tests, check-cran.sh.

SparkQA · 2017-04-27T14:20:40Z

Test build #76226 has finished for PR 17783 at commit 79857c9.

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

felixcheung · 2017-04-27T17:19:27Z

R/pkg/R/functions.R

+#' ## 3                       FALSE
+#' }
+#' @note not since 2.3.0
+setMethod("not",


I think this conflict with testthat..

It is, but testthat::not is deprecated anyway.

felixcheung · 2017-04-27T17:19:48Z

R/pkg/R/column.R

+#' ##3           FALSE
+#' }
+#' @note ! since 2.3.0
+setMethod("!",


does this not conflict with any existing R operator?

! is S4 generic. It is not different than & and | which we already support. I believe it hasn't been added so far just because it didn't fit into autogenerate template.

Do you have any thoughts about providing an example output in the docs. I see it makes Jenkins unhappy

R/column.R:325:5: style: Commented code should be removed.

but I believe this is an internal requirement, not something enforced by CRAN.

Note: I cannot reproduce this with local env (lintr 1.0). How is it checked on Jenkins?

which lintr? the current release is 0.2.0?
but I don't think we have a pattern for including output in example doc.

I think you could try

#' # (x = y) (x <=> y)

or

#' (x = y) (x <=> y)

https://cran.r-project.org/web/packages/lintr/, do we use something else here?

SparkQA · 2017-04-27T18:26:10Z

Test build #76235 has finished for PR 17783 at commit 39c9720.

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

SparkQA · 2017-04-27T19:21:11Z

Test build #76237 has finished for PR 17783 at commit a443a84.

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

felixcheung · 2017-04-28T04:21:43Z

R/pkg/R/column.R

@@ -67,8 +67,7 @@ operators <- list(
  "+" = "plus", "-" = "minus", "*" = "multiply", "/" = "divide", "%%" = "mod",
  "==" = "equalTo", ">" = "gt", "<" = "lt", "!=" = "notEqual", "<=" = "leq", ">=" = "geq",
  # we can not override `&&` and `||`, so use `&` and `|` instead
-  "&" = "and", "|" = "or", #, "!" = "unary_$bang"
-  "^" = "pow"
+  "&" = "and", "|" = "or", "^" = "pow"


what happens with #, ?

Since this PR implements ! I think it is just confusing, don't you? Not to mention that createOperator didn't work with !. I can restore it if you believe it serves some purpose.

actually, let me back track... it looks like "!" = "unary_$bang" has always been commented out. the #, does that
I agree we don't need to leave that comment here

felixcheung · 2017-04-28T04:22:50Z

R/pkg/R/column.R

+#' @note ! since 2.3.0
+setMethod("!",
+          signature(x = "Column"),
+          function(x) not(x))


maybe this should be single line?

setMethod("!", signature(x = "Column"), function(x) not(x))

felixcheung · 2017-04-28T04:26:25Z

R/pkg/inst/tests/testthat/test_sparkSQL.R

+
+  df <- as.DataFrame(data.frame(is_true = c(TRUE, FALSE, NA)))
+  expect_equal(
+    collect(select(df, alias(SparkR::not(df$is_true), "is_false"))),


we need SparkR:: here?

Initially I experienced some shading issues with testthat::not, but I cannot reproduce this any longer. Removed.

felixcheung · 2017-04-28T04:27:00Z

R/pkg/inst/tests/testthat/test_sparkSQL.R

@@ -1965,6 +1975,16 @@ test_that("filter() on a DataFrame", {

  # Test stats::filter is working
  #expect_true(is.ts(filter(1:100, rep(1, 3)))) # nolint
+
+  # test suites for %<=>%


can you move this before # Test stats::filter is working block

felixcheung · 2017-04-28T18:28:31Z

Interesting, I think we got 0.2 from his github https://github.com/jimhester/lintr

SparkQA · 2017-04-28T18:35:49Z

Test build #76279 has finished for PR 17783 at commit 47c4b41.

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

zero323 · 2017-04-28T18:41:54Z

Looks like the same package, just GitHub tags / releases are behind the CRAN. OK, let's remove example output for now and leave lintr issues for another day.

I wonder if we could actually remove \dontrun, push sparkR.session into \dontshow and still keep inside the CRAN time limits...

SparkQA · 2017-04-28T19:11:16Z

Test build #76280 has finished for PR 17783 at commit dc7c3dd.

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

SparkQA · 2017-04-28T20:26:24Z

Test build #76281 has finished for PR 17783 at commit 3004310.

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

SparkQA · 2017-04-28T21:06:03Z

Test build #76282 has finished for PR 17783 at commit 1788d6f.

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

SparkQA · 2017-04-28T23:07:08Z

Test build #76285 has finished for PR 17783 at commit a0c9222.

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

SparkQA · 2017-04-29T01:16:19Z

Test build #76290 has finished for PR 17783 at commit f83bcbd.

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

SparkQA · 2017-04-29T01:46:01Z

Test build #76291 has finished for PR 17783 at commit 457b9c2.

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

zero323 · 2017-04-29T05:17:13Z

Jenkins retest this please.

SparkQA · 2017-04-29T05:55:28Z

Test build #76295 has finished for PR 17783 at commit 457b9c2.

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

felixcheung · 2017-04-29T17:41:06Z

is this ready? please remove the [WIP] in the title

felixcheung · 2017-04-29T18:15:23Z

I wonder if we could actually remove \dontrun, push sparkR.session into \dontshow and still keep inside the CRAN time limits...

we could try but the number of methods and examples we have is much? higher than a lot of other packages....

zero323 · 2017-04-29T18:41:25Z

we could try but the number of methods and examples we have is much? higher than a lot of other packages....

Yeah... Maybe it is possible to hack docs generation process instead. Leave \dontrun for examples to keep CRAN happy but actually execute these blocks when we create docs?

is this ready? please remove the [WIP] in the title

I want to give it some thought first. I am still confused about the way in which CRAN check and roxygen generate docs for !. Between \name should not contain ! and checker don't recognizing that docs exist it feels hacky.

felixcheung · 2017-04-29T19:19:57Z

Sounds good - I didn't look very closely - what's the issue with ! doc

zero323 · 2017-04-29T19:40:00Z

In short, depending on how you set @rdname and @name we get warnings

 `prepare_Rd: not.Rd: \name should not contain !`

or

Undocumented S4 methods:
  generic '!' and siglist 'Column'

(not that we document any other Column operator) and both are considered failures in our build. Attaching these to Column docs seems to do the trick, but we put column function there as well, and it is not that clean.

I kind of know how to handle this by writing Rd files by hand, but it is not a solution here.

felixcheung · 2017-04-30T01:09:57Z

didn't it warn about @rdname column being lower cased? Not sure how later tests are passing.
what if we change both @rdname and @name?

#' @rdname not
#' @name not

zero323 · 2017-04-30T01:48:52Z

didn't it warn about @Rdname column being lower cased?

We use lower case column as @rdname for both column and Column-class so it is OK. We just have both in the same output file (and now ! as well).

I am pretty sure this is the one I used at the beginning, and it worked form me locally, but failed on Jenkins. I'll double check this but it feels like cargo cult programming.

SparkQA · 2017-04-30T02:26:04Z

Test build #76314 has finished for PR 17783 at commit 99f8dd2.

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

zero323 · 2017-04-30T04:43:41Z

Huh, seems to work after all.

felixcheung · 2017-04-30T05:25:32Z

I'm surprised @name %<=>% does too

felixcheung · 2017-04-30T05:31:16Z

R/pkg/R/column.R

+#' head(select(df, !column("x") > 0))
+#' }
+#' @note ! since 2.3.0
+#' @seealso \link{not}


we don't need this since @rdname not - they both go to the same html page

SparkQA · 2017-04-30T06:41:23Z

Test build #76316 has finished for PR 17783 at commit f44aa74.

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

SparkQA · 2017-04-30T20:50:41Z

Test build #76329 has finished for PR 17783 at commit 8ab9e0c.

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

felixcheung · 2017-05-01T05:08:36Z

merged to master

zero323 · 2017-05-01T05:21:28Z

Thanks.

zero323 changed the title ~~[SPARK-20490][SPARKR][WIP] Add R wrappers for eqNullSafe and ! / not.~~ [SPARK-20490][SPARKR][WIP] Add R wrappers for eqNullSafe and ! / not Apr 27, 2017

felixcheung reviewed Apr 27, 2017

View reviewed changes

felixcheung reviewed Apr 28, 2017

View reviewed changes

zero323 force-pushed the SPARK-20490 branch from 47c4b41 to dc7c3dd Compare April 28, 2017 18:34

zero323 force-pushed the SPARK-20490 branch from 3004310 to 1788d6f Compare April 28, 2017 20:31

zero323 force-pushed the SPARK-20490 branch from 1788d6f to a0c9222 Compare April 28, 2017 22:28

zero323 force-pushed the SPARK-20490 branch 2 times, most recently from f83bcbd to 457b9c2 Compare April 29, 2017 01:10

felixcheung reviewed Apr 30, 2017

View reviewed changes

zero323 changed the title ~~[SPARK-20490][SPARKR][WIP] Add R wrappers for eqNullSafe and ! / not~~ [SPARK-20490][SPARKR] Add R wrappers for eqNullSafe and ! / not Apr 30, 2017

zero323 added 13 commits April 30, 2017 22:08

Add null-safe equality operator (%<=>%)

5d10db9

Fix %<=>% details

13138e3

Implement not function and ! operator

5025344

Fix style

fb47620

Add not to masked names

38075bf

Add missing line at EOF

8b18e5b

Remove Spark:: prefix from not

412f4f0

Reorder filter suite

1c4cd9a

Remove example output(s) from not

af781a7

Remove example output and reformat ! and %<=>%

a2c24b5

Adjust name / rdname / aliases

f7b673a

Use not as @name and @Rdname for !

875c62f

Remove note

8ab9e0c

zero323 force-pushed the SPARK-20490 branch from f44aa74 to 8ab9e0c Compare April 30, 2017 20:15

felixcheung approved these changes May 1, 2017

View reviewed changes

asfgit closed this in 80e9cf1 May 1, 2017

zero323 deleted the SPARK-20490 branch May 8, 2017 09:08

[SPARK-20490][SPARKR] Add R wrappers for eqNullSafe and ! / not #17783

[SPARK-20490][SPARKR] Add R wrappers for eqNullSafe and ! / not #17783

Conversation

zero323 commented Apr 27, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zero323 Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Apr 27, 2017

SparkQA commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixcheung commented Apr 28, 2017 via email

SparkQA commented Apr 28, 2017

zero323 commented Apr 28, 2017 • edited Loading

SparkQA commented Apr 28, 2017

SparkQA commented Apr 28, 2017

SparkQA commented Apr 28, 2017

SparkQA commented Apr 28, 2017

SparkQA commented Apr 29, 2017

SparkQA commented Apr 29, 2017

zero323 commented Apr 29, 2017

SparkQA commented Apr 29, 2017

felixcheung commented Apr 29, 2017

felixcheung commented Apr 29, 2017 • edited Loading

zero323 commented Apr 29, 2017

felixcheung commented Apr 29, 2017 via email

zero323 commented Apr 29, 2017

felixcheung commented Apr 30, 2017

zero323 commented Apr 30, 2017

SparkQA commented Apr 30, 2017

zero323 commented Apr 30, 2017

felixcheung commented Apr 30, 2017

Choose a reason for hiding this comment

SparkQA commented Apr 30, 2017

SparkQA commented Apr 30, 2017

felixcheung commented May 1, 2017

zero323 commented May 1, 2017

zero323 commented Apr 27, 2017 •

edited

Loading

zero323 Apr 27, 2017 •

edited

Loading

zero323 commented Apr 28, 2017 •

edited

Loading

felixcheung commented Apr 29, 2017 •

edited

Loading