[GitHub] spark pull request #18158: [SPARK-20936][CORE]Lack of an important case abou...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/18158 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #18158: [SPARK-20936][CORE]Lack of an important case abou...
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119859774
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -461,19 +461,17 @@ class UtilsSuite extends SparkFunSuite with
ResetSystemProperties with Logging {
def assertResolves(before: String, after: String): Unit = {
// This should test only single paths
assume(before.split(",").length === 1)
- // Repeated invocations of resolveURI should yield the same result
def resolve(uri: String): String = Utils.resolveURI(uri).toString
+ assert(resolve(before) === after)
--- End diff --
Thanks for asking me. The initial change proposed looked testing several
duplicated test cases and also changing existing tests to me. I am fine with
the current status.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #18158: [SPARK-20936][CORE]Lack of an important case abou...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119858243
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -461,19 +461,17 @@ class UtilsSuite extends SparkFunSuite with
ResetSystemProperties with Logging {
def assertResolves(before: String, after: String): Unit = {
// This should test only single paths
assume(before.split(",").length === 1)
- // Repeated invocations of resolveURI should yield the same result
def resolve(uri: String): String = Utils.resolveURI(uri).toString
+ assert(resolve(before) === after)
--- End diff --
I think the change is OK @HyukjinKwon -- we do want to check that `before`
resolves to `after` and this case seems missed in this fixture. Or do I miss
something? The rest of the change is just for consistency
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #18158: [SPARK-20936][CORE]Lack of an important case abou...
Github user zuotingbing commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119535585
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -473,7 +474,7 @@ class UtilsSuite extends SparkFunSuite with
ResetSystemProperties with Logging {
val rawCwd = System.getProperty("user.dir")
val cwd = if (Utils.isWindows) s"/$rawCwd".replace("\\", "/") else
rawCwd
assertResolves("hdfs:/root/spark.jar", "hdfs:/root/spark.jar")
-assertResolves("hdfs:///root/spark.jar#app.jar",
"hdfs:/root/spark.jar#app.jar")
+assertResolves("hdfs:///root/spark.jar#app.jar",
"hdfs:///root/spark.jar#app.jar")
--- End diff --
ok, let me identify the cases between resolveURI and resolveURIs. what i
meant is we should check `assert(resolve(before) === after)` just in
`test("resolveURI")`
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #18158: [SPARK-20936][CORE]Lack of an important case abou...
Github user zuotingbing commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119534690
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -473,7 +474,7 @@ class UtilsSuite extends SparkFunSuite with
ResetSystemProperties with Logging {
val rawCwd = System.getProperty("user.dir")
val cwd = if (Utils.isWindows) s"/$rawCwd".replace("\\", "/") else
rawCwd
assertResolves("hdfs:/root/spark.jar", "hdfs:/root/spark.jar")
-assertResolves("hdfs:///root/spark.jar#app.jar",
"hdfs:/root/spark.jar#app.jar")
+assertResolves("hdfs:///root/spark.jar#app.jar",
"hdfs:///root/spark.jar#app.jar")
--- End diff --
sorry for my pool english. could we ask more people to review this and
get more opinions? @vanzin
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #18158: [SPARK-20936][CORE]Lack of an important case abou...
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119534247
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -473,7 +474,7 @@ class UtilsSuite extends SparkFunSuite with
ResetSystemProperties with Logging {
val rawCwd = System.getProperty("user.dir")
val cwd = if (Utils.isWindows) s"/$rawCwd".replace("\\", "/") else
rawCwd
assertResolves("hdfs:/root/spark.jar", "hdfs:/root/spark.jar")
-assertResolves("hdfs:///root/spark.jar#app.jar",
"hdfs:/root/spark.jar#app.jar")
+assertResolves("hdfs:///root/spark.jar#app.jar",
"hdfs:///root/spark.jar#app.jar")
--- End diff --
I think we do use it in `assert(new URI(Utils.resolveURIs(before)) === new
URI(after))`.
> different cases of `Utils.resolveURI`
I think we should identify the cases.
If the only reason is just meant to make the test code prettier but change
the existing tests for no reason, I would go -1.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #18158: [SPARK-20936][CORE]Lack of an important case abou...
Github user zuotingbing commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119533271
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -473,7 +474,7 @@ class UtilsSuite extends SparkFunSuite with
ResetSystemProperties with Logging {
val rawCwd = System.getProperty("user.dir")
val cwd = if (Utils.isWindows) s"/$rawCwd".replace("\\", "/") else
rawCwd
assertResolves("hdfs:/root/spark.jar", "hdfs:/root/spark.jar")
-assertResolves("hdfs:///root/spark.jar#app.jar",
"hdfs:/root/spark.jar#app.jar")
+assertResolves("hdfs:///root/spark.jar#app.jar",
"hdfs:///root/spark.jar#app.jar")
--- End diff --
what is the `before: String` targets in `assertResolves(`before: String`,
after: String)` ?
It seems to me that the different values of `before: String` test more
different cases of `Utils.resolveURI`. if we do not use `before: String` to
check, why add the params of `before: String`
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #18158: [SPARK-20936][CORE]Lack of an important case abou...
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119523325
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -473,7 +474,7 @@ class UtilsSuite extends SparkFunSuite with
ResetSystemProperties with Logging {
val rawCwd = System.getProperty("user.dir")
val cwd = if (Utils.isWindows) s"/$rawCwd".replace("\\", "/") else
rawCwd
assertResolves("hdfs:/root/spark.jar", "hdfs:/root/spark.jar")
-assertResolves("hdfs:///root/spark.jar#app.jar",
"hdfs:/root/spark.jar#app.jar")
+assertResolves("hdfs:///root/spark.jar#app.jar",
"hdfs:///root/spark.jar#app.jar")
--- End diff --
My question would be, what this test targets. It is probably okay to add
missing cases that does not change the existing tests but this changes the
existing regression test 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
