Skip to content
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.

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

[SPARK-8952] [SPARKR] - Wrap normalizePath calls with suppressWarnings #8343

Closed
wants to merge 1 commit into from

Conversation

lresende
Copy link
Member

This is based on @davies comment on SPARK-8952 which suggests to only call normalizePath() when path starts with '~'

@shivaram
Copy link
Contributor

Jenkins, ok to test

@SparkQA
Copy link

SparkQA commented Aug 21, 2015

Test build #41337 has finished for PR 8343 at commit 229a8df.

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

@shivaram
Copy link
Contributor

The change looks fine to me, but I am not sure we reached a consensus on the JIRA. The main problem is that right now normalizePath also handles relative paths and we'll lose that functionality with this change. So I'm not sure if this is the right fix yet -- it would be great if we could differentiate and handle local paths and HDFS paths.

Also a minor procedural point --- Could you title the pull request [SPARKR] [SPARK-8952] ... ? https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark has more details that might help.

@sun-rui
Copy link
Contributor

sun-rui commented Aug 26, 2015

For a short term solution, I think we can just use suppressWarnings(normalizePath(path)) to suppress the warning as is done in other places.

For a long term solution, we need to discuss if we really need this helping feature to normalize a path, as there is no such feature in Scala API. If we want this, need to differentiate and handle local paths and HDFS paths.

@lresende lresende changed the title SPARK-8952 - Restrict invocation of normalizePath() [SPARK-8952] [SPARKR] - Wrap normalizePath calls with suppressWarnings Aug 27, 2015
@lresende
Copy link
Member Author

Thanks for the comments @shivaram and @sun-rui. I have standardized in using supressWarnings for all invocations of normalizePath.

@SparkQA
Copy link

SparkQA commented Aug 27, 2015

Test build #41709 has finished for PR 8343 at commit f529bef.

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

Wrap normalizedPath calls with suppressWarnings to avoid issues
with s3 and other specific paths.
@SparkQA
Copy link

SparkQA commented Aug 28, 2015

Test build #41715 has finished for PR 8343 at commit 472c767.

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

@shivaram
Copy link
Contributor

Thanks @lresende -- LGTM

@shivaram
Copy link
Contributor

Jenkins, retest this please

@shivaram
Copy link
Contributor

@lresende I'm just retesting this as we merged a R style checker. I'm sure this PR should be fine, but just want to run this through to make sure things are working fine.

FYI @yu-iskw

@shivaram
Copy link
Contributor

@yu-iskw I actually an error in Jenkins which says. I guess this is from the PR that added na.omit to the NAMESPACE yesterday

Error in (function (classes, fdef, mtable)  : 
  unable to find an inherited method for function 'na.omit' for signature '"integer"'
Calls: lint_package ... ends -> as.igraph.es -> inherits -> na.omit -> <Anonymous>

@yu-iskw
Copy link
Contributor

yu-iskw commented Aug 28, 2015

@shivaram I see. I'll investigate the cause. Thank you for letting me know.

@shivaram
Copy link
Contributor

I think I found the problem. Our setGeneric for na.omit is wrong. It is being too restrictive. We need a diff which looks like

-           function(x, how = c("any", "all"), minNonNulls = NULL, cols = NULL) {
+           function(object, ...) {

I'll send a PR in a minute

@SparkQA
Copy link

SparkQA commented Aug 28, 2015

Test build #41718 has finished for PR 8343 at commit 472c767.

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

@shivaram
Copy link
Contributor

Jenkins, retest this please

@SparkQA
Copy link

SparkQA commented Aug 28, 2015

Test build #41735 has finished for PR 8343 at commit 472c767.

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

@shivaram
Copy link
Contributor

lint-r looks fine, Merging this.

asfgit pushed a commit that referenced this pull request Aug 28, 2015
This is based on davies comment on SPARK-8952 which suggests to only call normalizePath() when path starts with '~'

Author: Luciano Resende <lresende@apache.org>

Closes #8343 from lresende/SPARK-8952.

(cherry picked from commit 499e8e1)
Signed-off-by: Shivaram Venkataraman <shivaram@cs.berkeley.edu>
@asfgit asfgit closed this in 499e8e1 Aug 28, 2015
@lresende lresende deleted the SPARK-8952 branch August 28, 2015 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants