You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by cloud-fan <gi...@git.apache.org> on 2017/11/23 11:42:24 UTC
[GitHub] spark pull request #19799: [SPARK-17920][followup] simplify the schema file ...
GitHub user cloud-fan opened a pull request:
https://github.com/apache/spark/pull/19799
[SPARK-17920][followup] simplify the schema file creation in test
## What changes were proposed in this pull request?
a followup of https://github.com/apache/spark/pull/19779 , to simplify the file creation.
## How was this patch tested?
test only change
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloud-fan/spark minor
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19799.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 #19799
----
commit 71aeee57e32c2fd7cf9e4549fc0cc04b6712fb4e
Author: Wenchen Fan <we...@databricks.com>
Date: 2017-11-23T11:39:51Z
simplify the test
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19799: [SPARK-17920][followup] simplify the schema file ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19799#discussion_r152786374
--- Diff: sql/hive/src/test/scala/org/apache/spark/sql/hive/client/VersionsSuite.scala ---
@@ -862,17 +859,17 @@ class VersionsSuite extends SparkFunSuite with Logging {
| "logicalType": "decimal"
| }
| ]
- | } ]
+ | }]
|}
""".stripMargin
- val schemaUrl = s"""$schemaPath${File.separator}avroDecimal.avsc"""
- val schemaFile = new File(schemaPath, "avroDecimal.avsc")
+ val schemaFile = new File(dir, "avroDecimal.avsc")
val writer = new PrintWriter(schemaFile)
writer.write(avroSchema)
writer.close()
+ val schemaPath = schemaFile.getCanonicalPath
--- End diff --
what's the difference? we use `getCanonicalPath` in a lot of tests...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19799: [SPARK-17920][followup] simplify the schema file creatio...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19799
thanks, merging to master!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19799: [SPARK-17920][followup] simplify the schema file creatio...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19799
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84127/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19799: [SPARK-17920][followup] simplify the schema file ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19799#discussion_r152792399
--- Diff: sql/hive/src/test/scala/org/apache/spark/sql/hive/client/VersionsSuite.scala ---
@@ -862,17 +859,17 @@ class VersionsSuite extends SparkFunSuite with Logging {
| "logicalType": "decimal"
| }
| ]
- | } ]
+ | }]
|}
""".stripMargin
- val schemaUrl = s"""$schemaPath${File.separator}avroDecimal.avsc"""
- val schemaFile = new File(schemaPath, "avroDecimal.avsc")
+ val schemaFile = new File(dir, "avroDecimal.avsc")
val writer = new PrintWriter(schemaFile)
writer.write(avroSchema)
writer.close()
+ val schemaPath = schemaFile.getCanonicalPath
--- End diff --
how does `toURI` help here? it will escape the separator for us?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19799: [SPARK-17920][followup] simplify the schema file ...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/19799#discussion_r152797634
--- Diff: sql/hive/src/test/scala/org/apache/spark/sql/hive/client/VersionsSuite.scala ---
@@ -862,17 +859,17 @@ class VersionsSuite extends SparkFunSuite with Logging {
| "logicalType": "decimal"
| }
| ]
- | } ]
+ | }]
|}
""".stripMargin
- val schemaUrl = s"""$schemaPath${File.separator}avroDecimal.avsc"""
- val schemaFile = new File(schemaPath, "avroDecimal.avsc")
+ val schemaFile = new File(dir, "avroDecimal.avsc")
val writer = new PrintWriter(schemaFile)
writer.write(avroSchema)
writer.close()
+ val schemaPath = schemaFile.getCanonicalPath
--- End diff --
Sure, feel free to ask to test in the future as well. I can easily test.
Yes .. here:
```scala
scala> val path = new File("C:\\a\\b\\c").getCanonicalPath
path: String = C:\a\b\c
scala> sql(s"SELECT '$path'").show()
+-----+
|C:c|
+-----+
|C:c|
+-----+
scala> val path = new File("C:\\a\\b\\c").toURI.toString
path: String = file:/C:/a/b/c
scala> sql(s"SELECT '$path'").show()
+--------------+
|file:/C:/a/b/c|
+--------------+
|file:/C:/a/b/c|
+--------------+
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19799: [SPARK-17920][followup] simplify the schema file creatio...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19799
**[Test build #84127 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84127/testReport)** for PR 19799 at commit [`71aeee5`](https://github.com/apache/spark/commit/71aeee57e32c2fd7cf9e4549fc0cc04b6712fb4e).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19799: [SPARK-17920][followup] simplify the schema file creatio...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19799
**[Test build #84127 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84127/testReport)** for PR 19799 at commit [`71aeee5`](https://github.com/apache/spark/commit/71aeee57e32c2fd7cf9e4549fc0cc04b6712fb4e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19799: [SPARK-17920][followup] simplify the schema file ...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/19799#discussion_r152796192
--- Diff: sql/hive/src/test/scala/org/apache/spark/sql/hive/client/VersionsSuite.scala ---
@@ -862,17 +859,17 @@ class VersionsSuite extends SparkFunSuite with Logging {
| "logicalType": "decimal"
| }
| ]
- | } ]
+ | }]
|}
""".stripMargin
- val schemaUrl = s"""$schemaPath${File.separator}avroDecimal.avsc"""
- val schemaFile = new File(schemaPath, "avroDecimal.avsc")
+ val schemaFile = new File(dir, "avroDecimal.avsc")
val writer = new PrintWriter(schemaFile)
writer.write(avroSchema)
writer.close()
+ val schemaPath = schemaFile.getCanonicalPath
--- End diff --
For example, `/` in `file:/C:/a/b/c` is fine but problem seems about `\` in `C:\a\b\c`. I believe there are few issues open about this.
For example, on Windows:
```
scala> sql("select '\\\\'").show()
17/11/23 05:04:37 WARN SizeEstimator: Failed to check whether UseCompressedOops
is set; assuming yes
+---+
| \|
+---+
| \|
+---+
scala> sql("select '\\'").show()
org.apache.spark.sql.catalyst.parser.ParseException:
no viable alternative at input 'select ''(line 1, pos 7)
== SQL ==
select '\'
-------^^^
at org.apache.spark.sql.catalyst.parser.ParseException.withCommand(ParseDriver
.scala:239)
at org.apache.spark.sql.catalyst.parser.AbstractSqlParser.parse(ParseDriver.sc
ala:115)
at org.apache.spark.sql.execution.SparkSqlParser.parse(SparkSqlParser.scala:48
)
at org.apache.spark.sql.catalyst.parser.AbstractSqlParser.parsePlan(ParseDrive
r.scala:69)
at org.apache.spark.sql.SparkSession.sql(SparkSession.scala:638)
... 49 elided
scala> sql("select '/'").show()
+---+
| /|
+---+
| /|
+---+
scala> sql("select '//'").show()
+---+
| //|
+---+
| //|
+---+
```
So ... I was thinking using `toURI`
```scala
scala> new File("C:\\a\\b\\c").toURI.toString
res6: String = file:/C:/a/b/c
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19799: [SPARK-17920][followup] simplify the schema file ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/19799
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19799: [SPARK-17920][followup] simplify the schema file ...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/19799#discussion_r152788814
--- Diff: sql/hive/src/test/scala/org/apache/spark/sql/hive/client/VersionsSuite.scala ---
@@ -862,17 +859,17 @@ class VersionsSuite extends SparkFunSuite with Logging {
| "logicalType": "decimal"
| }
| ]
- | } ]
+ | }]
|}
""".stripMargin
- val schemaUrl = s"""$schemaPath${File.separator}avroDecimal.avsc"""
- val schemaFile = new File(schemaPath, "avroDecimal.avsc")
+ val schemaFile = new File(dir, "avroDecimal.avsc")
val writer = new PrintWriter(schemaFile)
writer.write(avroSchema)
writer.close()
+ val schemaPath = schemaFile.getCanonicalPath
--- End diff --
Ah, just for the test on Windows. Windows paths are like `C:\a\b\c` does not look going though the parser as is. It looks requiring escaping it .. Probably the easiest way is using URI to easily make the tests pass on Windows and Linux.
I have fixed those tests to pass on Windows few times. Up to my knowledge, there is a weak promise that Spark can run on Windows - https://spark.apache.org/docs/latest/#downloading. IMHO, it might be a bit better to use URI always when we remember this, and if those tests are not specific to path forms ..
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19799: [SPARK-17920][followup] simplify the schema file ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19799#discussion_r152848072
--- Diff: sql/hive/src/test/scala/org/apache/spark/sql/hive/client/VersionsSuite.scala ---
@@ -862,17 +859,17 @@ class VersionsSuite extends SparkFunSuite with Logging {
| "logicalType": "decimal"
| }
| ]
- | } ]
+ | }]
|}
""".stripMargin
- val schemaUrl = s"""$schemaPath${File.separator}avroDecimal.avsc"""
- val schemaFile = new File(schemaPath, "avroDecimal.avsc")
+ val schemaFile = new File(dir, "avroDecimal.avsc")
val writer = new PrintWriter(schemaFile)
writer.write(avroSchema)
writer.close()
+ val schemaPath = schemaFile.getCanonicalPath
--- End diff --
ah good to know this. Let's do a holistic review and fix all of them at once.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19799: [SPARK-17920][followup] simplify the schema file ...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/19799#discussion_r152799632
--- Diff: sql/hive/src/test/scala/org/apache/spark/sql/hive/client/VersionsSuite.scala ---
@@ -862,17 +859,17 @@ class VersionsSuite extends SparkFunSuite with Logging {
| "logicalType": "decimal"
| }
| ]
- | } ]
+ | }]
|}
""".stripMargin
- val schemaUrl = s"""$schemaPath${File.separator}avroDecimal.avsc"""
- val schemaFile = new File(schemaPath, "avroDecimal.avsc")
+ val schemaFile = new File(dir, "avroDecimal.avsc")
val writer = new PrintWriter(schemaFile)
writer.write(avroSchema)
writer.close()
+ val schemaPath = schemaFile.getCanonicalPath
--- End diff --
Leaving it as is is okay to me actually. I can fix them in a batch with more descriptions and explanation next time. We don't run Scala tests on Windows anyway as a requirement.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19799: [SPARK-17920][followup] simplify the schema file creatio...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19799
cc @vinodkc @gatorsmile
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19799: [SPARK-17920][followup] simplify the schema file ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19799#discussion_r152796692
--- Diff: sql/hive/src/test/scala/org/apache/spark/sql/hive/client/VersionsSuite.scala ---
@@ -862,17 +859,17 @@ class VersionsSuite extends SparkFunSuite with Logging {
| "logicalType": "decimal"
| }
| ]
- | } ]
+ | }]
|}
""".stripMargin
- val schemaUrl = s"""$schemaPath${File.separator}avroDecimal.avsc"""
- val schemaFile = new File(schemaPath, "avroDecimal.avsc")
+ val schemaFile = new File(dir, "avroDecimal.avsc")
val writer = new PrintWriter(schemaFile)
writer.write(avroSchema)
writer.close()
+ val schemaPath = schemaFile.getCanonicalPath
--- End diff --
Sorry I don't have a windowns machine to test, do you mean `new File("C:\\a\\b\\c").toURI.toString` is different from `new File("C:\\a\\b\\c").getCanonicalPath` excluding the `file:` prefix?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19799: [SPARK-17920][followup] simplify the schema file ...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/19799#discussion_r152781264
--- Diff: sql/hive/src/test/scala/org/apache/spark/sql/hive/client/VersionsSuite.scala ---
@@ -862,17 +859,17 @@ class VersionsSuite extends SparkFunSuite with Logging {
| "logicalType": "decimal"
| }
| ]
- | } ]
+ | }]
|}
""".stripMargin
- val schemaUrl = s"""$schemaPath${File.separator}avroDecimal.avsc"""
- val schemaFile = new File(schemaPath, "avroDecimal.avsc")
+ val schemaFile = new File(dir, "avroDecimal.avsc")
val writer = new PrintWriter(schemaFile)
writer.write(avroSchema)
writer.close()
+ val schemaPath = schemaFile.getCanonicalPath
--- End diff --
Could we be able to just use `toURI` instead of `getCanonicalPath`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19799: [SPARK-17920][followup] simplify the schema file creatio...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/19799
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19799: [SPARK-17920][followup] simplify the schema file creatio...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19799
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19799: [SPARK-17920][followup] simplify the schema file ...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/19799#discussion_r152790827
--- Diff: sql/hive/src/test/scala/org/apache/spark/sql/hive/client/VersionsSuite.scala ---
@@ -862,17 +859,17 @@ class VersionsSuite extends SparkFunSuite with Logging {
| "logicalType": "decimal"
| }
| ]
- | } ]
+ | }]
|}
""".stripMargin
- val schemaUrl = s"""$schemaPath${File.separator}avroDecimal.avsc"""
- val schemaFile = new File(schemaPath, "avroDecimal.avsc")
+ val schemaFile = new File(dir, "avroDecimal.avsc")
val writer = new PrintWriter(schemaFile)
writer.write(avroSchema)
writer.close()
+ val schemaPath = schemaFile.getCanonicalPath
--- End diff --
I just ran a test on Windows to double check:
Build started: [SQL] `org.apache.spark.sql.hive.client.VersionsSuite` [](https://ci.appveyor.com/project/spark-test/spark/branch/E6B8B497-CBC7-4978-A985-79302759F300)
Diff: https://github.com/apache/spark/compare/master...spark-test:E6B8B497-CBC7-4978-A985-79302759F300
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org