Skip to content

[SPARK-8603] [sparkR] In windows, Incorrect file seperator passed to … #7025

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pcp-dev wants to merge 1 commit into from
Closed

[SPARK-8603] [sparkR] In windows, Incorrect file seperator passed to … #7025

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion R/pkg/R/client.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,19 @@ determineSparkSubmitBin <- function() {
}
sparkSubmitBinName
}
# R supports both file separator in the file path (Unix : / and Windows: \) irrespective of the operating system
# but when passing file path to Java or script program, it has to be converted according to operating system

determinefileSeperator <- function() {
if (.Platform$OS.type == "unix") {
Copy link
Contributor

@JoshRosen JoshRosen Jan 13, 2016

Choose a reason for hiding this comment

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

Is anything which is not Unix guaranteed to be Windows? It seems like Windows is actually the special case here, so can we check its platform directly rather than assuming "not unix" is Windows?

Copy link
Contributor

@sun-rui sun-rui May 11, 2016

Choose a reason for hiding this comment

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

According to https://stat.ethz.ch/R-manual/R-devel/library/base/html/Platform.html, .Platform$OS.type have only two options: "unix" or "windows".

Copy link
Member

@HyukjinKwon HyukjinKwon May 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@sun-rui @JoshRosen I have a window machine. Although I am not used to R, if changing the condition to .Platform$OS.type != "windows" and rebasing are only things to be done, I can make a PR based on this. (The author might have to be @prakashpc though).

Copy link
Contributor

@sun-rui sun-rui May 18, 2016

Choose a reason for hiding this comment

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

@HyukjinKwon, cool, thanks! Could you double-check if this issue exists on Windows ,and this PR is the right fix. What I am wondering is that why system2 can't handle path separator since system2 is meant to be portable.

Copy link
Member

@HyukjinKwon HyukjinKwon May 18, 2016

Choose a reason for hiding this comment

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

@sun-rui I will try to test and then submit a PR. Thank you!

fileSeperator <- .Platform$file.sep
} else {
# .Platform$file.sep contains "/" for windows too
# http://www.inside-r.org/r-doc/base/file.path
fileSeperator <- "\\"
}
fileSeperator
}

generateSparkSubmitArgs <- function(args, sparkHome, jars, sparkSubmitOpts, packages) {
if (jars != "") {
Expand All @@ -59,7 +72,8 @@ generateSparkSubmitArgs <- function(args, sparkHome, jars, sparkSubmitOpts, pack
launchBackend <- function(args, sparkHome, jars, sparkSubmitOpts, packages) {
sparkSubmitBinName <- determineSparkSubmitBin()
if (sparkHome != "") {
sparkSubmitBin <- file.path(sparkHome, "bin", sparkSubmitBinName)
fileSeperator <- determinefileSeperator()
sparkSubmitBin <- file.path(sparkHome, "bin", sparkSubmitBinName, fsep = fileSeperator)
} else {
sparkSubmitBin <- sparkSubmitBinName
}
Expand Down