[GitHub] spark pull request #20464: [SPARK-23291][SQL][R] R's substr should not reduc...

2018-03-07 Thread asfgit 
Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/20464


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20464: [SPARK-23291][SQL][R] R's substr should not reduc...

2018-03-06 Thread viirya 
Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/20464#discussion_r172751184
  
--- Diff: docs/sparkr.md ---
@@ -663,3 +663,7 @@ You can inspect the search path in R with 
[`search()`](https://stat.ethz.ch/R-ma
  - The `stringsAsFactors` parameter was previously ignored with `collect`, 
for example, in `collect(createDataFrame(iris), stringsAsFactors = TRUE))`. It 
has been corrected.
  - For `summary`, option for statistics to compute has been added. Its 
output is changed from that from `describe`.
  - A warning can be raised if versions of SparkR package and the Spark JVM 
do not match.
+
+## Upgrading to Spark 2.4.0
+
+ - The `start` parameter of `substr` method was wrongly subtracted by one, 
previously. In other words, the index specified by `start` parameter was 
considered as 0-base. This can lead to inconsistent substring results and also 
does not match with the behaviour with `substr` in R. It has been fixed so the 
`start` parameter of `substr` method is now 1-base, e.g., `substr(df$a, 2, 5)` 
should be changed to `substr(df$a, 1, 4)`.
--- End diff --

Yes. Added.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20464: [SPARK-23291][SQL][R] R's substr should not reduc...

2018-03-06 Thread felixcheung 
Github user felixcheung commented on a diff in the pull request:

https://github.com/apache/spark/pull/20464#discussion_r172750404
  
--- Diff: docs/sparkr.md ---
@@ -663,3 +663,7 @@ You can inspect the search path in R with 
[`search()`](https://stat.ethz.ch/R-ma
  - The `stringsAsFactors` parameter was previously ignored with `collect`, 
for example, in `collect(createDataFrame(iris), stringsAsFactors = TRUE))`. It 
has been corrected.
  - For `summary`, option for statistics to compute has been added. Its 
output is changed from that from `describe`.
  - A warning can be raised if versions of SparkR package and the Spark JVM 
do not match.
+
+## Upgrading to Spark 2.4.0
+
+ - The `start` parameter of `substr` method was wrongly subtracted by one, 
previously. In other words, the index specified by `start` parameter was 
considered as 0-base. This can lead to inconsistent substring results and also 
does not match with the behaviour with `substr` in R. It has been fixed so the 
`start` parameter of `substr` method is now 1-base, e.g., `substr(df$a, 2, 5)` 
should be changed to `substr(df$a, 1, 4)`.
--- End diff --

could you add
`method is now 1-base, e.g., therefore to get the same result as 
substr(df$a, 2, 5), it should be changed to substr(df$a, 1, 4)`


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20464: [SPARK-23291][SQL][R] R's substr should not reduc...

2018-03-06 Thread viirya 
Github user viirya closed the pull request at:

https://github.com/apache/spark/pull/20464


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20464: [SPARK-23291][SQL][R] R's substr should not reduc...

2018-03-06 Thread viirya 
GitHub user viirya reopened a pull request:

https://github.com/apache/spark/pull/20464

[SPARK-23291][SQL][R] R's substr should not reduce starting position by 1 
when calling Scala API

## What changes were proposed in this pull request?

Seems R's substr API treats Scala substr API as zero based and so subtracts 
the given starting position by 1.

Because Scala's substr API also accepts zero-based starting position 
(treated as the first element), so the current R's substr test results are 
correct as they all use 1 as starting positions.

## How was this patch tested?

Modified tests.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/viirya/spark-1 SPARK-23291

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/20464.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #20464


commit a2ffdc14ebfa67656e3598f0a0a0131f18f98aa5
Author: Liang-Chi Hsieh 
Date:   2018-02-01T03:10:53Z

R's substr should not reduce starting position by 1 when calling Scala API.

commit 95c8a4e48e8f760bb9ca0df844136d19452521d7
Author: Liang-Chi Hsieh 
Date:   2018-02-01T09:02:16Z

Add a note to migration guide of R doc.

commit d994d76d45e474b3e4a31fff8250c30efef6a757
Author: Liang-Chi Hsieh 
Date:   2018-02-02T08:30:43Z

Fix doc.

commit 0ebdf74942e0894bfaf6cbede4c03fd3f5d26411
Author: Liang-Chi Hsieh 
Date:   2018-03-06T04:54:48Z

Improve doc.




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20464: [SPARK-23291][SQL][R] R's substr should not reduc...

2018-03-05 Thread viirya 
Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/20464#discussion_r172409837
  
--- Diff: R/pkg/R/column.R ---
@@ -169,7 +169,7 @@ setMethod("alias",
 #' @note substr since 1.4.0
 setMethod("substr", signature(x = "Column"),
   function(x, start, stop) {
-jc <- callJMethod(x@jc, "substr", as.integer(start - 1), 
as.integer(stop - start + 1))
+jc <- callJMethod(x@jc, "substr", as.integer(start), 
as.integer(stop - start + 1))
--- End diff --

Added to the func doc.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20464: [SPARK-23291][SQL][R] R's substr should not reduc...

2018-03-05 Thread viirya 
Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/20464#discussion_r172406939
  
--- Diff: R/pkg/R/column.R ---
@@ -169,7 +169,7 @@ setMethod("alias",
 #' @note substr since 1.4.0
 setMethod("substr", signature(x = "Column"),
   function(x, start, stop) {
-jc <- callJMethod(x@jc, "substr", as.integer(start - 1), 
as.integer(stop - start + 1))
+jc <- callJMethod(x@jc, "substr", as.integer(start), 
as.integer(stop - start + 1))
--- End diff --

I think you mean 1-based,


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20464: [SPARK-23291][SQL][R] R's substr should not reduc...

2018-03-03 Thread felixcheung 
Github user felixcheung commented on a diff in the pull request:

https://github.com/apache/spark/pull/20464#discussion_r172033037
  
--- Diff: docs/sparkr.md ---
@@ -663,3 +663,7 @@ You can inspect the search path in R with 
[`search()`](https://stat.ethz.ch/R-ma
  - The `stringsAsFactors` parameter was previously ignored with `collect`, 
for example, in `collect(createDataFrame(iris), stringsAsFactors = TRUE))`. It 
has been corrected.
  - For `summary`, option for statistics to compute has been added. Its 
output is changed from that from `describe`.
  - A warning can be raised if versions of SparkR package and the Spark JVM 
do not match.
+
+## Upgrading to Spark 2.4.0
+
+ - The `start` parameter of `substr` method was wrongly subtracted by one, 
previously. This can lead to inconsistent substring results and also does not 
match with the behaviour with `substr` in R. It has been corrected.
--- End diff --

2. in the migration guide we should give a concrete example with non-0 
start index, eg.
`substr(df$a, 1, 6)` should be changed to `substr(df$a, 0, 5)`


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20464: [SPARK-23291][SQL][R] R's substr should not reduc...

2018-03-03 Thread felixcheung 
Github user felixcheung commented on a diff in the pull request:

https://github.com/apache/spark/pull/20464#discussion_r172033021
  
--- Diff: R/pkg/R/column.R ---
@@ -169,7 +169,7 @@ setMethod("alias",
 #' @note substr since 1.4.0
 setMethod("substr", signature(x = "Column"),
   function(x, start, stop) {
-jc <- callJMethod(x@jc, "substr", as.integer(start - 1), 
as.integer(stop - start + 1))
+jc <- callJMethod(x@jc, "substr", as.integer(start), 
as.integer(stop - start + 1))
--- End diff --

I think we should do two things:
1. add to the func doc that the `start` param should be 0-base and to add 
to the example with the result
`collect(select(df, substr(df$a, 0, 5))) # this should give you...`



---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20464: [SPARK-23291][SQL][R] R's substr should not reduc...

2018-02-02 Thread viirya 
Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/20464#discussion_r165584849
  
--- Diff: docs/sparkr.md ---
@@ -663,3 +663,7 @@ You can inspect the search path in R with 
[`search()`](https://stat.ethz.ch/R-ma
  - The `stringsAsFactors` parameter was previously ignored with `collect`, 
for example, in `collect(createDataFrame(iris), stringsAsFactors = TRUE))`. It 
has been corrected.
  - For `summary`, option for statistics to compute has been added. Its 
output is changed from that from `describe`.
  - A warning can be raised if versions of SparkR package and the Spark JVM 
do not match.
+
+## Upgrading to Spark 2.4.0
+
+ - The first parameter of `substr` method was wrongly subtracted by one, 
previously. This can lead to inconsistent substring results and also does not 
match with the behaviour with `substr` in R. It has been corrected.
--- End diff --

Fixed.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20464: [SPARK-23291][SQL][R] R's substr should not reduc...

2018-02-02 Thread viirya 
Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/20464#discussion_r165584627
  
--- Diff: R/pkg/R/column.R ---
@@ -169,7 +169,7 @@ setMethod("alias",
 #' @note substr since 1.4.0
 setMethod("substr", signature(x = "Column"),
   function(x, start, stop) {
-jc <- callJMethod(x@jc, "substr", as.integer(start - 1), 
as.integer(stop - start + 1))
+jc <- callJMethod(x@jc, "substr", as.integer(start), 
as.integer(stop - start + 1))
--- End diff --

> is there a way to make the behavior the same before this change for any 
caller calling substr with common index like 0

Should we keep the behavior when calling substr with 0 as start index?

```R
> df <- createDataFrame(list(list(a="abcdef")))
> collect(select(df, substr(df$a, 0, 5)))
  substring(a, -1, 6)
1   f
> substr("abcdef", 0, 5)
[1] "abcde"
```

I think the previous behavior is pretty unreasonable..


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20464: [SPARK-23291][SQL][R] R's substr should not reduc...

2018-02-02 Thread viirya 
Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/20464#discussion_r165582806
  
--- Diff: R/pkg/R/column.R ---
@@ -169,7 +169,7 @@ setMethod("alias",
 #' @note substr since 1.4.0
 setMethod("substr", signature(x = "Column"),
   function(x, start, stop) {
-jc <- callJMethod(x@jc, "substr", as.integer(start - 1), 
as.integer(stop - start + 1))
+jc <- callJMethod(x@jc, "substr", as.integer(start), 
as.integer(stop - start + 1))
--- End diff --

> * why consider other changes as a follow up and not here? [#20464 
(comment)](https://github.com/apache/spark/pull/20464#issuecomment-362162014)

Just because I think it is another issue regarding 0/negative indices. I 
can deal it here if you strongly feel it is better.




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20464: [SPARK-23291][SQL][R] R's substr should not reduc...

2018-02-01 Thread felixcheung 
Github user felixcheung commented on a diff in the pull request:

https://github.com/apache/spark/pull/20464#discussion_r165571511
  
--- Diff: R/pkg/R/column.R ---
@@ -169,7 +169,7 @@ setMethod("alias",
 #' @note substr since 1.4.0
 setMethod("substr", signature(x = "Column"),
   function(x, start, stop) {
-jc <- callJMethod(x@jc, "substr", as.integer(start - 1), 
as.integer(stop - start + 1))
+jc <- callJMethod(x@jc, "substr", as.integer(start), 
as.integer(stop - start + 1))
--- End diff --

question:
- is there a way to make the behavior the same before this change for any 
caller calling substr with common index like 0
- why consider other changes as a follow up and not here? 
https://github.com/apache/spark/pull/20464#issuecomment-362162014



---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20464: [SPARK-23291][SQL][R] R's substr should not reduc...

2018-02-01 Thread felixcheung 
Github user felixcheung commented on a diff in the pull request:

https://github.com/apache/spark/pull/20464#discussion_r165570529
  
--- Diff: docs/sparkr.md ---
@@ -663,3 +663,7 @@ You can inspect the search path in R with 
[`search()`](https://stat.ethz.ch/R-ma
  - The `stringsAsFactors` parameter was previously ignored with `collect`, 
for example, in `collect(createDataFrame(iris), stringsAsFactors = TRUE))`. It 
has been corrected.
  - For `summary`, option for statistics to compute has been added. Its 
output is changed from that from `describe`.
  - A warning can be raised if versions of SparkR package and the Spark JVM 
do not match.
+
+## Upgrading to Spark 2.4.0
+
+ - The first parameter of `substr` method was wrongly subtracted by one, 
previously. This can lead to inconsistent substring results and also does not 
match with the behaviour with `substr` in R. It has been corrected.
--- End diff --

instead of  `The first parameter of ` -> `The ``start`` parameter of...`


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20464: [SPARK-23291][SQL][R] R's substr should not reduc...

2018-01-31 Thread viirya 
Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/20464#discussion_r165271143
  
--- Diff: R/pkg/R/column.R ---
@@ -169,7 +169,7 @@ setMethod("alias",
 #' @note substr since 1.4.0
 setMethod("substr", signature(x = "Column"),
   function(x, start, stop) {
-jc <- callJMethod(x@jc, "substr", as.integer(start - 1), 
as.integer(stop - start + 1))
+jc <- callJMethod(x@jc, "substr", as.integer(start), 
as.integer(stop - start + 1))
--- End diff --

This API behavior should be considered as wrong and performs 
inconsistently. Because for starting position 1, we get substring from 1st 
element, but for position 2, we still get the substring from 1. So we will get 
the following inconsistent results:

```R
> collect(select(df, substr(df$a, 1, 5)))
  substring(a, 0, 5)
1  abcde
> collect(select(df, substr(df$a, 2, 5)))
  substring(a, 1, 4)
1   abcd
```

For such change, we might need to add a note in the doc as @HyukjinKwon 
suggested.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20464: [SPARK-23291][SQL][R] R's substr should not reduc...

2018-01-31 Thread felixcheung 
Github user felixcheung commented on a diff in the pull request:

https://github.com/apache/spark/pull/20464#discussion_r165263961
  
--- Diff: R/pkg/R/column.R ---
@@ -169,7 +169,7 @@ setMethod("alias",
 #' @note substr since 1.4.0
 setMethod("substr", signature(x = "Column"),
   function(x, start, stop) {
-jc <- callJMethod(x@jc, "substr", as.integer(start - 1), 
as.integer(stop - start + 1))
+jc <- callJMethod(x@jc, "substr", as.integer(start), 
as.integer(stop - start + 1))
--- End diff --

I'm a bit concern with changing this. As you can see it's been like this 
from the very beginning...


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20464: [SPARK-23291][SQL][R] R's substr should not reduc...

2018-01-31 Thread viirya 
GitHub user viirya opened a pull request:

https://github.com/apache/spark/pull/20464

[SPARK-23291][SQL][R] R's substr should not reduce starting position by 1 
when calling Scala API

## What changes were proposed in this pull request?

Seems R's substr API treats Scala substr API as zero based and so subtracts 
the given starting position by 1. Because SQL's substr also accepts zero-based 
starting position, this also causes incorrect results when the starting 
position is greater than 1, but we don't have related tests in R.

## How was this patch tested?

Modified tests.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/viirya/spark-1 SPARK-23291

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/20464.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #20464


commit a2ffdc14ebfa67656e3598f0a0a0131f18f98aa5
Author: Liang-Chi Hsieh 
Date:   2018-02-01T03:10:53Z

R's substr should not reduce starting position by 1 when calling Scala API.




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org