You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by tpoterba <gi...@git.apache.org> on 2017/05/16 19:40:32 UTC
[GitHub] spark pull request #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeField...
GitHub user tpoterba opened a pull request:
https://github.com/apache/spark/pull/18005
[SPARK-20773][SQL] ParquetWriteSupport.writeFields is quadratic in number of fields
Fix quadratic List indexing in ParquetWriteSupport.
I noticed this function while profiling some code with today. It showed up as a significant factor in a table with twenty columns; with hundreds of columns, it could dominate any other function call.
## What changes were proposed in this pull request?
The writeFields method iterates from 0 until number of fields, indexing into rootFieldWriters for each element. rootFieldWriters is a List, so indexing is a linear operation. The complexity of the writeFields method is thus quadratic in the number of fields.
Solution: explicitly convert rootFieldWriters to Array (implicitly converted to WrappedArray) for constant-time indexing.
## How was this patch tested?
This is a one-line change for performance reasons.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tpoterba/spark tpoterba-patch-1
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/18005.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 #18005
----
commit e13ce9f85af8a0f87af09e49de04cfda30bf9180
Author: Tim Poterba <tp...@gmail.com>
Date: 2017-05-16T19:29:46Z
Fix quadratic List indexing in ParquetWriteSupport
Fix quadratic List indexing in ParquetWriteSupport.
Minimal solution is to convert rootFieldWriters to a WrappedArray, which has O(1) indexing, and restores complexity to linear.
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeField...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/18005#discussion_r117029865
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetWriteSupport.scala ---
@@ -90,7 +90,7 @@ private[parquet] class ParquetWriteSupport extends WriteSupport[InternalRow] wit
}
- this.rootFieldWriters = schema.map(_.dataType).map(makeWriter)
+ this.rootFieldWriters = schema.map(_.dataType).map(makeWriter).toArray[ValueWriter]
--- End diff --
Either call toIndexedSeq or make the rootFieldWriters an Array. Both are fine.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/18005
LGTM pending jenkins
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18005
**[Test build #76983 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/76983/testReport)** for PR 18005 at commit [`e13ce9f`](https://github.com/apache/spark/commit/e13ce9f85af8a0f87af09e49de04cfda30bf9180).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18005
Can one of the admins verify this patch?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/18005
Can you also make sure that we do not use a `Seq` for struct writing?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18005
Merged build finished. Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18005
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/76983/
Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18005
**[Test build #77018 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/77018/testReport)** for PR 18005 at commit [`72c5487`](https://github.com/apache/spark/commit/72c5487d39424fe82c1c6030246ca891355b176f).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeField...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/18005#discussion_r117029723
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetWriteSupport.scala ---
@@ -90,7 +90,7 @@ private[parquet] class ParquetWriteSupport extends WriteSupport[InternalRow] wit
}
- this.rootFieldWriters = schema.map(_.dataType).map(makeWriter).toArray
+ this.rootFieldWriters = schema.map(_.dataType).map(makeWriter).toArray[ValueWriter]
--- End diff --
Either call `toIndexedSeq` or make the `rootFieldWriters` an Array. Both are fine.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by tpoterba <gi...@git.apache.org>.
Github user tpoterba commented on the issue:
https://github.com/apache/spark/pull/18005
I used this script to generate random CSV files:
```python
import uuid
import sys
try:
print('args = ' + str(sys.argv))
filename = sys.argv[1]
cols = int(sys.argv[2])
rows = int(sys.argv[3])
if len(sys.argv) != 4 or cols <= 0 or rows <= 0:
raise RuntimeError()
except Exception as e:
raise RuntimeError('Usage: gen_text_file.py <filename> <cols> <rows>')
rand_to_gen = (cols + 7) / 8
with open(filename, 'w') as f:
f.write(','.join('col%d' % i for i in range(cols)))
f.write('\n')
for i in range(rows):
if (i % 10000 == 0):
print('wrote %d lines' % i)
rands = [x[i:i+4] for i in range(8) for x in [uuid.uuid4().hex for _ in range(rand_to_gen)]]
f.write(','.join(rands[:cols]))
f.write('\n')
```
I generated files that were all the same size on disk with different dimensions (cols x rows):
10x18M
20x9M
30x6M
60x3M
150x1200K
300x600K
Here's what I tried to do to them:
```python
>>> spark.read.csv(text_file).write.mode('overwrite').parquet(parquet_path)
````
The 10, 20, 30-column files all took between 40s to 1m to complete on 2 cores of my laptop. 60 and up never completed, and actually crashed the java process -- I had to kill it with `kill -9`.
At one point for the 60-column table, I got a "GC overhead limit exceeded" OOM from the parquet writer (the error suggested that parquet was doing something silly trying to use dictionary encoding for random values, but I haven't figured out how to turn that off). I could be conflating this crash with one we encountered a few months ago, where Spark crashed because Catalyst generated bytecode larger than 64k for dataframes with a large schema.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by tpoterba <gi...@git.apache.org>.
Github user tpoterba commented on the issue:
https://github.com/apache/spark/pull/18005
Addressed comments.
I tried to get some benchmark stats for this code:
```python
spark.read.csv(text_file).write.mode('overwrite').parquet(parquet_path)
```
I wanted to see the performance improvement for files with various numbers of columns/rows that were all 1.5G. However, I didn't see much of a difference with <30 columns and catalyst blew up when I tried ~50 columns (I wanted to go up to several hundred)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by tpoterba <gi...@git.apache.org>.
Github user tpoterba commented on the issue:
https://github.com/apache/spark/pull/18005
Others on my team suggest that the >64k bytecode issue has been fixed already (and ported to a 2.1 maintenance release as well)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/18005
What do you mean by catalyst blew up?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/18005
ok to test
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/18005
LGTM - merging to master/2.2. Thanks!
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18005
Merged build finished. Test PASSed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18005
**[Test build #76983 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/76983/testReport)** for PR 18005 at commit [`e13ce9f`](https://github.com/apache/spark/commit/e13ce9f85af8a0f87af09e49de04cfda30bf9180).
* This patch **fails to build**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the issue:
https://github.com/apache/spark/pull/18005
It might be nice to explicitly use the type `IndexedSeq[ValueWriter]` for `rootFieldWriters` (up on line 61 of this file) since that would capture the intent behind using an Array and would maybe help prevent regression during refactoring.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18005
**[Test build #77018 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/77018/testReport)** for PR 18005 at commit [`72c5487`](https://github.com/apache/spark/commit/72c5487d39424fe82c1c6030246ca891355b176f).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18005
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/77018/
Test PASSed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeField...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/18005
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18005: [SPARK-20773][SQL] ParquetWriteSupport.writeFields is qu...
Posted by tpoterba <gi...@git.apache.org>.
Github user tpoterba commented on the issue:
https://github.com/apache/spark/pull/18005
Yeah, I can change that - I do hate the standard IndexedSeq implementation (Vector) though, and want to make sure that the collection is actually a WrappedArray.
I've actually done more than make a one-line change using the Github UI now, and will update with performance benchmarks + a successful build.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org