You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by MaxGekk <gi...@git.apache.org> on 2018/11/01 12:30:23 UTC
[GitHub] spark pull request #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson par...
GitHub user MaxGekk opened a pull request:
https://github.com/apache/spark/pull/22920
[SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in the encoding JSON benchmarks
## What changes were proposed in this pull request?
The #21909 introduced an optimisation for `count()` which impacted on JSON benchmarks related to encoding. The benchmarks should demonstrate that performance does not degrade significantly when `encoding` is set explicitly to `UTF-8`. After the empty schema related changes, the `count()` action does not trigger creation of Jackson parser at all, and the benchmarks became useless. In the PR, I propose to prevent projection pushdown and create Jackson parser forcibly.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MaxGekk/spark-1 json-benchmark-follow-up
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22920.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 #22920
----
commit f31e438f291cae18bdd1b7d6443f8f9b23d82b2c
Author: Maxim Gekk <ma...@...>
Date: 2018-11-01T11:24:26Z
Re-run the benchmark before changes
commit c7d5cc433bf98025e5bd5776b271d4af328893f0
Author: Maxim Gekk <ma...@...>
Date: 2018-11-01T12:15:00Z
Invoke Jackson parser in benchmarks
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22920
**[Test build #98411 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98411/testReport)** for PR 22920 at commit [`9e32447`](https://github.com/apache/spark/commit/9e3244755e2a3a647ed84cd1d56fbe0a4033caaa).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22920
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/98351/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22920
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/98356/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22920: [SPARK-25931][SQL] Benchmarking creation of Jackson pars...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22920
**[Test build #98424 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98424/testReport)** for PR 22920 at commit [`4ecc7e0`](https://github.com/apache/spark/commit/4ecc7e03dac6148047608af3b0199348302fce74).
* 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 #22920: [SPARK-25931][SQL] Benchmarking creation of Jackson pars...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22920
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 issue #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22920
**[Test build #98356 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98356/testReport)** for PR 22920 at commit [`c7d5cc4`](https://github.com/apache/spark/commit/c7d5cc433bf98025e5bd5776b271d4af328893f0).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22920
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 issue #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22920
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22920: [SPARK-25931][SQL] Benchmarking creation of Jackson pars...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/22920
Thank you, @MaxGekk and @HyukjinKwon !
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22920: [SPARK-25931][SQL] Benchmarking creation of Jackson pars...
Posted by MaxGekk <gi...@git.apache.org>.
Github user MaxGekk commented on the issue:
https://github.com/apache/spark/pull/22920
@dongjoon-hyun Thank you for re-running the benchmarks on EC2, and @HyukjinKwon for review.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in ...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/22920
@MaxGekk .
1. Could you review https://github.com/MaxGekk/spark-1/pull/14 and merge that?
I recovered the iteration number and update the result on EC2.
2. Also, please create a new JIRA issue. If you reuse the JIRA issue like this, the fix version and the content becomes less coherent. SPARK-24959 is 2.4.0 and this is for 3.0, isn't it?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson par...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/22920#discussion_r230059288
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/JsonBenchmark.scala ---
@@ -86,6 +86,7 @@ object JSONBenchmark extends SqlBasedBenchmark {
spark.read
.schema(schema)
.json(path.getAbsolutePath)
+ .filter((_: Row) => true)
--- End diff --
@MaxGekk . This is another benchmark case, isn't it?
We should have different benchmark cases for these.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson par...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/22920#discussion_r230138764
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/JsonBenchmark.scala ---
@@ -86,6 +86,7 @@ object JSONBenchmark extends SqlBasedBenchmark {
spark.read
.schema(schema)
.json(path.getAbsolutePath)
+ .filter((_: Row) => true)
--- End diff --
BTW, how do you want to call old and new test cases?
- For old case, we can give a new name.
- For new case, `JSON per-line parsing:` looks not a little bit accurate because we have filters now.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22920: [SPARK-25931][SQL] Benchmarking creation of Jackson pars...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22920
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/98424/
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 #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson par...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/22920#discussion_r230059513
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/JsonBenchmark.scala ---
@@ -86,6 +86,7 @@ object JSONBenchmark extends SqlBasedBenchmark {
spark.read
.schema(schema)
.json(path.getAbsolutePath)
+ .filter((_: Row) => true)
--- End diff --
This is not a follow-up. Please create another JIRA to add these test cases.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson par...
Posted by MaxGekk <gi...@git.apache.org>.
Github user MaxGekk commented on a diff in the pull request:
https://github.com/apache/spark/pull/22920#discussion_r230095357
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/JsonBenchmark.scala ---
@@ -86,6 +86,7 @@ object JSONBenchmark extends SqlBasedBenchmark {
spark.read
.schema(schema)
.json(path.getAbsolutePath)
+ .filter((_: Row) => true)
--- End diff --
> This is another benchmark case, isn't it?
Originally I added the benchmark to check how specifying of `encoding` impacts on performance. This worked well till #21909 . Currently the benchmark just test how fast JSON datasource can create empty rows (in the case of `count()`) which is checked by another benchmark.
I believe this PR is just follow up of #21909 which must include the changes proposed in the PR.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22920
**[Test build #98411 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98411/testReport)** for PR 22920 at commit [`9e32447`](https://github.com/apache/spark/commit/9e3244755e2a3a647ed84cd1d56fbe0a4033caaa).
* 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 #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22920
**[Test build #98351 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98351/testReport)** for PR 22920 at commit [`c7d5cc4`](https://github.com/apache/spark/commit/c7d5cc433bf98025e5bd5776b271d4af328893f0).
* This patch **fails SparkR unit 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 #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22920
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22920
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/98411/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22920
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22920
**[Test build #98424 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98424/testReport)** for PR 22920 at commit [`4ecc7e0`](https://github.com/apache/spark/commit/4ecc7e03dac6148047608af3b0199348302fce74).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22920: [SPARK-25931][SQL] Benchmarking creation of Jacks...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/22920
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22920
**[Test build #98356 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98356/testReport)** for PR 22920 at commit [`c7d5cc4`](https://github.com/apache/spark/commit/c7d5cc433bf98025e5bd5776b271d4af328893f0).
* This patch **fails SparkR unit 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 #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22920
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson par...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/22920#discussion_r230136993
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/JsonBenchmark.scala ---
@@ -86,6 +86,7 @@ object JSONBenchmark extends SqlBasedBenchmark {
spark.read
.schema(schema)
.json(path.getAbsolutePath)
+ .filter((_: Row) => true)
--- End diff --
@MaxGekk . In your PR, you already show the side effect via benchmark .
What I mean is both test cases are meaningful and worth to have. :) And, we need to compare both results in the future release.
In any way, please create new different benchmark cases for this PR.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22920
**[Test build #98351 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98351/testReport)** for PR 22920 at commit [`c7d5cc4`](https://github.com/apache/spark/commit/c7d5cc433bf98025e5bd5776b271d4af328893f0).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson parser in ...
Posted by MaxGekk <gi...@git.apache.org>.
Github user MaxGekk commented on the issue:
https://github.com/apache/spark/pull/22920
jenkins, retest this, please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22920: [SPARK-24959][SQL][FOLLOWUP] Creating Jackson par...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/22920#discussion_r230546330
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/JsonBenchmark.scala ---
@@ -158,26 +166,78 @@ object JSONBenchmark extends SqlBasedBenchmark {
val ds = spark.read.schema(schema).json(path.getAbsolutePath)
- benchmark.addCase(s"Select $colsNum columns + count()", 3) { _ =>
+ benchmark.addCase(s"Select $colsNum columns + count()", numIters) { _ =>
ds.select("*").filter((_: Row) => true).count()
}
- benchmark.addCase(s"Select 1 column + count()", 3) { _ =>
+ benchmark.addCase(s"Select 1 column + count()", numIters) { _ =>
ds.select($"col1").filter((_: Row) => true).count()
}
- benchmark.addCase(s"count()", 3) { _ =>
+ benchmark.addCase(s"count()", numIters) { _ =>
ds.count()
}
benchmark.run()
}
}
+ def jsonParserCreation(rowsNum: Int, numIters: Int): Unit = {
+ val benchmark = new Benchmark("creation of JSON parser per line", rowsNum, output = output)
+
+ withTempPath { path =>
+ prepareDataInfo(benchmark)
+
+ val shortColumnPath = path.getAbsolutePath + "/short"
+ val shortSchema = writeShortColumn(shortColumnPath, rowsNum)
+
+ val wideColumnPath = path.getAbsolutePath + "/wide"
+ val wideSchema = writeWideColumn(wideColumnPath, rowsNum)
+
+ benchmark.addCase("Short column without encoding", numIters) { _ =>
+ spark.read
+ .schema(shortSchema)
+ .json(shortColumnPath)
+ .filter((_: Row) => true)
+ .count()
+ }
+
+ benchmark.addCase("Short column with UTF-8", numIters) { _ =>
+ spark.read
+ .option("encoding", "UTF-8")
+ .schema(shortSchema)
+ .json(shortColumnPath)
+ .filter((_: Row) => true)
+ .count()
+ }
+
+ benchmark.addCase("Wide column without encoding", numIters) { _ =>
+ spark.read
+ .schema(wideSchema)
+ .json(wideColumnPath)
+ .filter((_: Row) => true)
+ .count()
+ }
+
+ benchmark.addCase("Wide column with UTF-8", numIters) { _ =>
+ spark.read
+ .option("encoding", "UTF-8")
+ .schema(wideSchema)
+ .json(wideColumnPath)
+ .filter((_: Row) => true)
+ .count()
+ }
+
+ benchmark.run()
+ }
+ }
+
override def runBenchmarkSuite(mainArgs: Array[String]): Unit = {
+ val numIters = 2
--- End diff --
Thank you for updating, @MaxGekk .
Do we have a reason to decrease this value from 3 to 2 in this PR?
If this is to reduce the time, let's keep the original value.
This benchmark is not executed frequently.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org