You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by HyukjinKwon <gi...@git.apache.org> on 2016/09/05 11:43:19 UTC
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
GitHub user HyukjinKwon opened a pull request:
https://github.com/apache/spark/pull/14960
[WIP][SPARK-17339][SPARKR][CORE] Fix Windows path issues in SparkContext causing the tests failures in SparkR on Windows
## What changes were proposed in this pull request?
This PR fixes the Windows path issues in several APIs. Please refer https://issues.apache.org/jira/browse/SPARK-17339 for more details.
## How was this patch tested?
Tests via AppVeyor CI - https://ci.appveyor.com/project/HyukjinKwon/spark/build/69-SPARK-17339-after
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/HyukjinKwon/spark SPARK-17339
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/14960.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 #14960
----
commit 071168290b6cfd332c02dd3e44415c7e069dbb46
Author: hyukjinkwon <gu...@gmail.com>
Date: 2016-09-05T09:49:15Z
Avoid direct use of URI for file paths in Windows
commit f42c182cc3d7eaf863ccab7c52b59ddc3e12c17a
Author: hyukjinkwon <gu...@gmail.com>
Date: 2016-09-05T11:37:15Z
Handle Windows specific path issues
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path issues...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14960
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/64936/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14960
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14960
**[Test build #65026 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/65026/consoleFull)** for PR 14960 at commit [`41aaaf1`](https://github.com/apache/spark/commit/41aaaf127e949af7563024c1584567a177295409).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77525722
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
I will double check and will look into this deeper. I didn't notice we have that test anyway. Thanks for pointing this out.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77573002
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
The other option is to of course just use the Hadoop `Path` class and do something like `new Path(path).toURI` -- I think they handle `C:/` correctly. I don't know if this affects other functionality though (like SPARK-11227) and we should check with @sarutak
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77568657
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
I see. Actually, it seems it has to be `C:\Users\appveyor\AppData\Local\Temp\1\RtmpkzTq6t\glm78c146111b1.tmp` for path without the `slash`.
It seems the problem is, in many classes such as `RWrappers`, there are the usages of `new Path(path, "rMetadata").toString`.
It seems this converts the path, for example,
from
```
C:\Users\appveyor\AppData\Local\Temp\1\RtmpkzTq6t\glm78c146111b1.tmp
```
to
```
C:/Users/appveyor/AppData/Local/Temp/1/RtmpkzTq6t/glm78c146111b1.tmp
```
So, for example, it goes through `textFile` -> `hadoopFile` -> `Utils.resolveURI`. Here the given path is `C:/Users/appveyor/AppData/Local/Temp/1/RtmpkzTq6t/glm78c146111b1.tmp` but it seems `Utils.resolveURI` does not handle this case.
So, this returns this as it is.
```scala
...
val uri = new URI(path)
if (uri.getScheme() != null) { // here it just passes becuase `C:/.../...` has the scheme `C`.
return uri
}
...
```
and it ends up with an exception as below:
```scala
java.io.IOException: No FileSystem for scheme: C
at org.apache.hadoop.fs.FileSystem.getFileSystemClass(FileSystem.java:2421)
at org.apache.hadoop.fs.FileSystem.createFileSystem(FileSystem.java:2428)
at org.apache.hadoop.fs.FileSystem.access$200(FileSystem.java:88)
at org.apache.hadoop.fs.FileSystem$Cache.getInternal(FileSystem.java:2467)
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and ...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77747323
--- Diff: core/src/main/scala/org/apache/spark/SparkContext.scala ---
@@ -992,7 +992,7 @@ class SparkContext(config: SparkConf) extends Logging with ExecutorAllocationCli
// This is a hack to enforce loading hdfs-site.xml.
// See SPARK-11227 for details.
- FileSystem.get(new URI(path), hadoopConfiguration)
+ FileSystem.get(new Path(path).toUri, hadoopConfiguration)
--- End diff --
As it is known that is hacky and ugly, maybe we can make this separate to another issue (although I am careful to say this)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77543535
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
When we pass in a path from R to Scala we first call `normalizePath` and then pass the output of that to JVM [1, 2].
So the strings in R look something like
```
> d <- normalizePath("./README.md")
> d
[1] "C:\\Users\\shivaram\\Downloads\\spark-1-sparkr-win-tests-fix\\spark-1-sparkr-win-tests-fix\\README.md"
```
Now there is an option in `normalizePath` to use the `/` instead for Windows alone. So then the output looks something like
```
> d <- normalizePath("./README.md", winslash="/")
> d
[1] "C:/Users/shivaram/Downloads/spark-1-sparkr-win-tests-fix/spark-1-sparkr-win-tests-fix/README.md"
```
If the second one works better with `resolveURI` we can just change the R code to pass in this option
[1] https://github.com/apache/spark/blob/6d86403d8b252776effcddd71338b4d21a224f9b/R/pkg/R/context.R#L75
[2] https://github.com/apache/spark/blob/6d86403d8b252776effcddd71338b4d21a224f9b/R/pkg/R/mllib.R#L905
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77551553
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
Thanks both for your help really!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/14960
Yeap, I quickly fixed and re-ran :). Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14960
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/65030/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77574745
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
Let me please just use `new Path(...).toUri` directly to deal with this if it seems okay.
I just tried to fix `resolveURI` to use `new Path(path).toUri` instead of `new URI(path)` but I found it breaks existing tests for `resolveURI`. It seems parsing special characters differently, for example , `#`
```
"hdfs:/root/spark.jar[%23]app.jar" did not equal "hdfs:/root/spark.jar[#]app.jar"
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77575448
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
O.K, let's fix the other problem of `resolveURI` in another PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path issues...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14960
**[Test build #64936 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/64936/consoleFull)** for PR 14960 at commit [`f42c182`](https://github.com/apache/spark/commit/f42c182cc3d7eaf863ccab7c52b59ddc3e12c17a).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77573054
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
Yeap, meanwhile, I will try to use that and run tests. Thanks for your quick feedback..
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path issues...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/14960
I would like to make sure that this pass both AppVeyor CI for SparkR and Jenkins ones. So, I will remove `[WIP]` if the tests go okay for both.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and ...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77694632
--- Diff: core/src/main/scala/org/apache/spark/SparkContext.scala ---
@@ -992,7 +992,7 @@ class SparkContext(config: SparkConf) extends Logging with ExecutorAllocationCli
// This is a hack to enforce loading hdfs-site.xml.
// See SPARK-11227 for details.
- FileSystem.get(new URI(path), hadoopConfiguration)
+ FileSystem.get(new Path(path).toUri, hadoopConfiguration)
--- End diff --
I *think* that's handled upstream in SparkSession/SQLContext.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14960
**[Test build #64972 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/64972/consoleFull)** for PR 14960 at commit [`790d5b2`](https://github.com/apache/spark/commit/790d5b2304473555d1edf113f9bbee3034134fac).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/14960
I re-run the test after this commit - https://ci.appveyor.com/project/HyukjinKwon/spark/build/81-SPARK-17339-fix-r
Let's wait and see :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on the issue:
https://github.com/apache/spark/pull/14960
I found we can replace `FileSystem.get` in `SparkContext#hadoopFile` and `SparkContext.newAPIHadoopFile` with `FileSystem.getLocal` like `SparkContext#hadoopRDD` so once they are replaced, we need not discuss the case of comma-separated file list.
@HyukjinKwon You can replace them in this PR or not do it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77568884
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
So, I am a bit confused I should still change R path.
```scala
new Path(path, "rMetadata").toString
```
is being used pretty much in those wrappers.
Would this make sense if this case (handling `C:/../..`) is treated within `Utils.resolveURI` cc - @sarutak ? If so, I will try to neat up the codes more.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on the issue:
https://github.com/apache/spark/pull/14960
LGTM. Merging this into `master` and `branch-2.0`. Thanks @HyukjinKwon !
And thanks @shivaram and @felixcheung for the review !
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77574419
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
The way @shivaram mentioned works well and doesn't affect SPARK-11227.
@HyukjinKwon You can fix this problem with the way but if you will fix `resolveURI`, adding new test cases to `UtilsSuite` is desireble.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14960
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/64972/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77544947
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
For the LDA test we can try a fix that looks like https://github.com/shivaram/spark-1/commit/5aaa322c6c4b567df6c89686fe2e2a65f55e1540#diff-51c07c6af7649f6c021e5a5438e31a4f
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/spark/pull/14960
seems to fail to build:
```
[INFO] Compiling 468 Scala sources and 74 Java sources to C:\projects\spark\core\target\scala-2.11\classes...
[ERROR] C:\projects\spark\core\src\main\scala\org\apache\spark\SparkContext.scala:995: type mismatch;
found : org.apache.spark.SparkConf
required: org.apache.hadoop.conf.Configuration
[ERROR] FileSystem.getLocal(conf)
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14960
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path issues...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/14960
Yup, finally we got a green - https://ci.appveyor.com/project/HyukjinKwon/spark/build/77-SPARK-17339-fix-r!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/14960
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77572574
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
I see - Unless there is a reason not to handle such paths it will be good to make the change to convert `C:/../..` to valid URIs as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and ...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77747258
--- Diff: core/src/main/scala/org/apache/spark/SparkContext.scala ---
@@ -992,7 +992,7 @@ class SparkContext(config: SparkConf) extends Logging with ExecutorAllocationCli
// This is a hack to enforce loading hdfs-site.xml.
// See SPARK-11227 for details.
- FileSystem.get(new URI(path), hadoopConfiguration)
+ FileSystem.get(new Path(path).toUri, hadoopConfiguration)
--- End diff --
Hm.. I didn't know it supports comma separated path. BTW, we still can use `spark.sparkContext.textFile(..)` though.
I took a look and it seems okay though (but it's ugly and hacky).
If the first given path is okay, it seems working fine. It looks only `getScheme` and `getAuth` in `FileSystem.get(..)` (I track down the `FileSystem.get(..)` and related function calls.)
So, iff the first path is correct, it seems `getAuthority` and `getScheme` give a correct ones to get a file system.
For example, the path `http://localhost:8080/a/b,http://localhost:8081/c/d` parses the URI as below:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and ...
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77751910
--- Diff: core/src/main/scala/org/apache/spark/SparkContext.scala ---
@@ -992,7 +992,7 @@ class SparkContext(config: SparkConf) extends Logging with ExecutorAllocationCli
// This is a hack to enforce loading hdfs-site.xml.
// See SPARK-11227 for details.
- FileSystem.get(new URI(path), hadoopConfiguration)
+ FileSystem.get(new Path(path).toUri, hadoopConfiguration)
--- End diff --
Yeah I'm not sure what part of the URI we are using here. If its just the scheme, authority then I think its fine to use that from the first path. FWIW there is a method in Hadoop to parse comma separated path strings but its private [1].
IMHO this problem existed even before this PR so I'm fine not fixing it here if thats okay with @sarutak
[1] https://hadoop.apache.org/docs/r2.7.1/api/src-html/org/apache/hadoop/mapred/FileInputFormat.html#line.467
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77529504
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
<img width="583" alt="2016-09-05 11 25 33" src="https://cloud.githubusercontent.com/assets/6477701/18250760/17550748-73c0-11e6-8c22-119fd42e4150.png">
I suspect the tests do not pass.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/14960
Sure! I will tomorrow.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77526122
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
Here is the original test failures FYI - https://gist.github.com/shivaram/7693df7bd54dc81e2e7d1ce296c41134
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/14960
@sarutak Ah, I will do this here. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and ...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77747489
--- Diff: core/src/main/scala/org/apache/spark/SparkContext.scala ---
@@ -992,7 +992,7 @@ class SparkContext(config: SparkConf) extends Logging with ExecutorAllocationCli
// This is a hack to enforce loading hdfs-site.xml.
// See SPARK-11227 for details.
- FileSystem.get(new URI(path), hadoopConfiguration)
+ FileSystem.get(new Path(path).toUri, hadoopConfiguration)
--- End diff --
cc - @sarutak WDYT? is my understanding correct?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path issues...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14960
**[Test build #64936 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/64936/consoleFull)** for PR 14960 at commit [`f42c182`](https://github.com/apache/spark/commit/f42c182cc3d7eaf863ccab7c52b59ddc3e12c17a).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and ...
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77665699
--- Diff: core/src/main/scala/org/apache/spark/SparkContext.scala ---
@@ -992,7 +992,7 @@ class SparkContext(config: SparkConf) extends Logging with ExecutorAllocationCli
// This is a hack to enforce loading hdfs-site.xml.
// See SPARK-11227 for details.
- FileSystem.get(new URI(path), hadoopConfiguration)
+ FileSystem.get(new Path(path).toUri, hadoopConfiguration)
--- End diff --
One minor question I had was how this would work with comma separated list of file names as we allow that in textFile (for ample at ehttps://github.com/HyukjinKwon/spark/blob/790d5b2304473555d1edf113f9bbee3034134fac/core/src/test/scala/org/apache/spark/SparkContextSuite.scala#L323)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77544303
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
BTW if we can get to the test errors in https://gist.github.com/HyukjinKwon/0c42b2c208e06c59525d91087252d9b0 it will be very cool. The only remaining errors are from the LDA tests and that is about not locating the test data file -- that seems like a different bug that we can fix in a follow up
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77525653
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
Hm.. yeap, Firstly, I didn't add this logic and run the test with this diff (https://github.com/HyukjinKwon/spark/compare/master...HyukjinKwon:a136206dad6011d6ad112f33417790ad3c6a9912) produced the output here
https://gist.github.com/HyukjinKwon/f3f9a36dde88028ca09fd417b6ce5c68
So, I corrected this (without knowing we are testing that case). with the diff here (https://github.com/HyukjinKwon/spark/compare/master...HyukjinKwon:b648e4f2d5aae072748a97550cfcf832c57d9315)
produced the output here, https://gist.github.com/HyukjinKwon/0c42b2c208e06c59525d91087252d9b0
This seems it reduces roughly 10ish test failures. So I thought this is a legitimate change.
I will double-check.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14960
**[Test build #65026 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/65026/consoleFull)** for PR 14960 at commit [`41aaaf1`](https://github.com/apache/spark/commit/41aaaf127e949af7563024c1584567a177295409).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14960
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/65026/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on the issue:
https://github.com/apache/spark/pull/14960
I noticed this PR is not able to be merged cleanly to `branch-2.0`.
@HyukjinKwon If you would like to merge this into `branch-2.0`, please feel free to open another PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14960
**[Test build #65030 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/65030/consoleFull)** for PR 14960 at commit [`41aaaf1`](https://github.com/apache/spark/commit/41aaaf127e949af7563024c1584567a177295409).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77510739
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
BTW, this logic is taken after Hadoop codes. Please let me know if there are equivalent functions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77531923
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
Ah, If I remember this correctly `C:/../..` works in Windows with R. Is this true or am i just imagining - cc @shivaram ?
So, R pass the paths as they as (e.g. `C:/.../..`) and it recognizes pathes as below:
<img width="387" alt="2016-09-05 11 45 07" src="https://cloud.githubusercontent.com/assets/6477701/18251218/d1db8b8a-73c2-11e6-8930-d6d9831c34b4.png">
and then produces some error cases?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path issues...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14960
**[Test build #64972 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/64972/consoleFull)** for PR 14960 at commit [`790d5b2`](https://github.com/apache/spark/commit/790d5b2304473555d1edf113f9bbee3034134fac).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on the issue:
https://github.com/apache/spark/pull/14960
Thanks @HyukjinKwon - LGTM but for a minor comment I had inline.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14960
Merged build finished. Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on the issue:
https://github.com/apache/spark/pull/14960
Looks like it passed ! LGTM pending our Jenkins tests
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on a diff in the pull request:
https://github.com/apache/spark/pull/14960#discussion_r77519854
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1900,7 +1900,20 @@ private[spark] object Utils extends Logging {
*/
def resolveURI(path: String): URI = {
try {
- val uri = new URI(path)
+ val osSafePath = if (Path.isWindowsAbsolutePath(path, false)) {
+ // Make sure C:/ part becomes /C/.
+ val windowsUri = new URI(path)
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
+ } else if (Path.isWindowsAbsolutePath(path, true)) {
+ // Make sure /C:/ part becomes /C/.
+ val windowsUri = new URI(path.substring(1))
+ val driveLetter = windowsUri.getScheme
+ s"/$driveLetter/${windowsUri.getSchemeSpecificPart()}"
--- End diff --
Is this change needed? I think `resolveURI` already treat Windows-style path.
https://github.com/apache/spark/blob/master/core/src/test/scala/org/apache/spark/util/UtilsSuite.scala#L398
Or, did you find another case which this method cannot treat properly?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14960
**[Test build #65030 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/65030/consoleFull)** for PR 14960 at commit [`41aaaf1`](https://github.com/apache/spark/commit/41aaaf127e949af7563024c1584567a177295409).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [WIP][SPARK-17339][SPARKR][CORE] Fix Windows path issues...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14960
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14960: [SPARK-17339][SPARKR][CORE] Fix some R tests and use Pat...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/14960
retest this please
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org