Skip to content

Conversation

@itholic
Copy link
Contributor

@itholic itholic commented Nov 9, 2022

What changes were proposed in this pull request?

The original PR to introduce the error class PATH_NOT_FOUND was reverted since it breaks the tests in different test env.

This PR proposes to restore it back.

Why are the changes needed?

Restoring the reverted changes with proper fix.

Does this PR introduce any user-facing change?

No

How was this patch tested?

The existing CI should pass.

@LuciferYang
Copy link
Contributor

Is the sparkr UTs failure is related to this one?

https://github.com/itholic/spark/actions/runs/3425639144/jobs/5708796073

══ Failed ══════════════════════════════════════════════════════════════════════
── 1. Failure ('test_sparkSQL.R:3993'): Call DataFrameWriter.load() API in Java 
`read.df("arbitrary_path")` threw an error with unexpected message.
Expected match: "Error in load : analysis error - Path does not exist"
Actual message: "Error in load : analysis error - [PATH_NOT_FOUND] Path does not exist: file:/__w/spark/spark/arbitrary_path."
Backtrace:
  1. testthat::expect_error(read.df("arbitrary_path"), "Error in load : analysis error - Path does not exist")
       at test_sparkSQL.R:3993:2
  6. SparkR::read.df("arbitrary_path")
  7. SparkR:::handledCallJMethod(read, "load")
  8. base::tryCatch(...)
  9. base (local) tryCatchList(expr, classes, parentenv, handlers)
 10. base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
 11. value[[3L]](cond)
 12. SparkR:::captureJVMException(e, method)

@LuciferYang
Copy link
Contributor

Looks like we need to refactor this case

test_that("Call DataFrameWriter.load() API in Java without path and check argument types", {
# This tests if the exception is thrown from JVM not from SparkR side.
# It makes sure that we can omit path argument in read.df API and then it calls
# DataFrameWriter.load() without path.
expect_error(read.df(source = "json"),
paste("Error in load : analysis error - Unable to infer schema for JSON.",
"It must be specified manually"))
expect_error(read.df("arbitrary_path"), "Error in load : analysis error - Path does not exist")
expect_error(read.json("arbitrary_path"), "Error in json : analysis error - Path does not exist")
expect_error(read.text("arbitrary_path"), "Error in text : analysis error - Path does not exist")
expect_error(read.orc("arbitrary_path"), "Error in orc : analysis error - Path does not exist")
expect_error(read.parquet("arbitrary_path"),
"Error in parquet : analysis error - Path does not exist")

@github-actions github-actions bot added the R label Nov 14, 2022
@itholic itholic changed the title [WIP][SPARK-40948][SQL][FOLLOWUP] Restore PATH_NOT_FOUND [SPARK-40948][SQL][FOLLOWUP] Restore PATH_NOT_FOUND Nov 22, 2022
@itholic itholic marked this pull request as ready for review November 22, 2022 06:51
expect_error(read.text("arbitrary_path"), "Error in text : analysis error - Path does not exist")
expect_error(read.orc("arbitrary_path"), "Error in orc : analysis error - Path does not exist")
expect_error(read.df("arbitrary_path"),
"Error in load : analysis error - \\[PATH_NOT_FOUND\\].*")
Copy link
Member

Choose a reason for hiding this comment

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

@HyukjinKwon @srielau @cloud-fan Are you ok with such changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: the arbitrary_path here is different per test environments, so I use regexp for the path string.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

Choose a reason for hiding this comment

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

thanks man

@MaxGekk
Copy link
Member

MaxGekk commented Nov 23, 2022

+1, LGTM. Merging to master.
Thank you, @itholic and @HyukjinKwon @cloud-fan @LuciferYang for review.

@MaxGekk MaxGekk closed this in 1781617 Nov 23, 2022
beliefer pushed a commit to beliefer/spark that referenced this pull request Dec 15, 2022
### What changes were proposed in this pull request?

The original PR to introduce the error class `PATH_NOT_FOUND` was reverted since it breaks the tests in different test env.

This PR proposes to restore it back.

### Why are the changes needed?

Restoring the reverted changes with proper fix.

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

The existing CI should pass.

Closes apache#38575 from itholic/SPARK-40948-followup.

Authored-by: itholic <[email protected]>
Signed-off-by: Max Gekk <[email protected]>
beliefer pushed a commit to beliefer/spark that referenced this pull request Dec 18, 2022
### What changes were proposed in this pull request?

The original PR to introduce the error class `PATH_NOT_FOUND` was reverted since it breaks the tests in different test env.

This PR proposes to restore it back.

### Why are the changes needed?

Restoring the reverted changes with proper fix.

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

The existing CI should pass.

Closes apache#38575 from itholic/SPARK-40948-followup.

Authored-by: itholic <[email protected]>
Signed-off-by: Max Gekk <[email protected]>
@itholic itholic deleted the SPARK-40948-followup branch April 22, 2023 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants