[SPARK-16814][SQL] Fix deprecated parquet constructor usage #14419

holdenk · 2016-07-30T07:10:18Z

What changes were proposed in this pull request?

Replace deprecated ParquetWriter with the new builders

How was this patch tested?

Existing tests

SparkQA · 2016-07-30T08:54:51Z

Test build #63039 has finished for PR 14419 at commit e10c147.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class ParquetWriterBuilder() extends

srowen · 2016-07-30T11:34:46Z

Do we know if this works across versions of Parquet that are still in play for Spark builds? I know we've got some YARN and Scala warnings that we can't adjust for that reason. Just double checking. @HyukjinKwon did you look at this too or am I imagining it?

HyukjinKwon · 2016-07-30T14:31:10Z

Thank you for cc me! Yes, actually I took a look for this as well and it seems okay (I was thinking doing this with other issues together).
If my understanding is correct, this would not affect actual writing with Parquet in Spark but just tests.

HyukjinKwon · 2016-07-30T16:11:23Z

Could I cc @liancheng who has more insight about this please?

holdenk · 2016-07-30T19:18:09Z

So yes as @HyukjinKwon says its just in the tests.
I figured this would be ok across the hadoop versions we target because I looked at the build deps files and they all listed parquet-hadoop-1.8.1 for the different hadoop profiles we build with (grep -r parquet-hadoop ./dev/deps/)

holdenk · 2016-07-30T19:21:44Z

@HyukjinKwon do you have a PR for this? If so I can go ahead and close this one (and lets maybe sync some on the deprecation stuff we are both cleaning up :))

HyukjinKwon · 2016-07-30T21:23:13Z

No, I don't :). I just meant I wanted to say that this fix is reasonable and the reason I didn't submit a PR has not been because there is potential incompatability. It's just been because just I was thinking doing this with other ones in the future. I like this PR :).

Sorry for confusing you.

rxin · 2016-07-30T21:53:36Z

...test/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetCompatibilityTest.scala

@@ -119,8 +119,19 @@ private[sql] object ParquetCompatibilityTest {
      metadata: Map[String, String],
      recordWriters: (RecordConsumer => Unit)*): Unit = {
    val messageType = MessageTypeParser.parseMessageType(schema)
-    val writeSupport = new DirectWriteSupport(messageType, metadata)
-    val parquetWriter = new ParquetWriter[RecordConsumer => Unit](new Path(path), writeSupport)
+    val testWriteSupport = new DirectWriteSupport(messageType, metadata)


can you explain what's going on here? This seems like pretty complicated.

Sure, so the parquetWriter constructors are deprecated now and its been replaced with a builder interface. For Avro and others there is a standard builder - but for sort "raw" formats you need to implement your own builder. This is equivalent to the old constructor we were using - you can see the deprecation in https://github.com/apache/parquet-mr/pull/199/files as well as how the builder interface ends up calling an equivalent (now protected) constructor. Also since our WriteSupport doesn't need to change based on the configuration we always return the same writesupport regardless of conf.

If it would be useful I can add some of this as a comment in the sourcecode.

@rxin Does this explanation make sense for you?

You might just go ahead and add it as a comment for good measure.

Isn't @Override the Java annotation? thought Scala needed @override but could be missing something.

I didn't know Java annotation passes Scala style checking. Should we consider adding a rule for this?

Sure, because I don't think it would actually cause the Scala compiler to verify it overrides. It's not illegal to use a Java annotation, just doesn't do anything in this case?

Filed here, https://issues.apache.org/jira/browse/SPARK-16877 but I will do some researches and take a look into this deeper first before opening a PR.

scala just do override, not even @OverRide.

Ack yeah that's what I meant, thank you.

…es since we just expose getWriteSupport)

SparkQA · 2016-07-31T04:04:10Z

Test build #63051 has finished for PR 14419 at commit 5e92e6f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

holdenk · 2016-07-31T23:52:02Z

@HyukjinKwon no worries - just wanted to make sure we weren't accidentally duplicating eachothers efforts :)

HyukjinKwon · 2016-08-01T00:19:53Z

...test/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetCompatibilityTest.scala

-    val writeSupport = new DirectWriteSupport(messageType, metadata)
-    val parquetWriter = new ParquetWriter[RecordConsumer => Unit](new Path(path), writeSupport)
+    val testWriteSupport = new DirectWriteSupport(messageType, metadata)
+    case class ParquetWriterBuilder() extends


One thing I am a bit wondering though is why this has to be case class instead of just class. Because it seems it is self-contained, so it seems not have to be serializable and it seems there is no pattern matching with this. Just curious.

Thats a good point, they probably don't need to be case classes (just happened to write that way when I started the code) - I'll switch them to regular classes.

…ctor-usage

…f github.com:holdenk/spark into SPARK-16814-fix-deprecated-parquet-constructor-usage

SparkQA · 2016-08-01T03:40:33Z

Test build #63062 has finished for PR 14419 at commit 2067b63.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2016-08-03T11:13:03Z

...test/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetCompatibilityTest.scala

+    val testWriteSupport = new DirectWriteSupport(messageType, metadata)
+    class ParquetWriterBuilder() extends
+        ParquetWriter.Builder[RecordConsumer => Unit, ParquetWriterBuilder](new Path(path)) {
+      @Override def getWriteSupport(conf: org.apache.hadoop.conf.Configuration) = {


Import Configuration? and maybe just write the trivial body of both methods inline without braces on one line. Just personal taste really, to keep this anonymous class compact

…ctor-usage

…ing the builder pattern

SparkQA · 2016-08-03T21:49:58Z

Test build #63183 has finished for PR 14419 at commit 1a37b8f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2016-08-04T00:09:01Z

Merged to master

holdenk added 4 commits July 29, 2016 13:46

start refactoring to builder

9fa8c8f

Merge branch 'master' into fix-deprecated-parquet-constructor-usage

b41a057

Use the builder

1ba7643

Cleanup some other deprecated parquet writer constructors

e10c147

rxin reviewed Jul 30, 2016
View reviewed changes

Simplify a little bit (remove final val writeSupport from inner class…

5e92e6f

…es since we just expose getWriteSupport)

holdenk changed the title ~~[SPARK-16814][SQL][WIP] Fix deprecated parquet constructor usage~~ [SPARK-16814][SQL] Fix deprecated parquet constructor usage Aug 1, 2016

HyukjinKwon reviewed Aug 1, 2016
View reviewed changes

holdenk added 3 commits July 31, 2016 18:19

Merge branch 'master' into SPARK-16814-fix-deprecated-parquet-constru…

844bbc9

…ctor-usage

These don't need to be case classes - switch to regular

6f35d6b

Merge branch 'SPARK-16814-fix-deprecated-parquet-constructor-usage' o…

2067b63

…f github.com:holdenk/spark into SPARK-16814-fix-deprecated-parquet-constructor-usage

srowen reviewed Aug 3, 2016
View reviewed changes

holdenk added 3 commits August 3, 2016 11:44

Merge branch 'master' into SPARK-16814-fix-deprecated-parquet-constru…

518f6db

…ctor-usage

Switch to correct override and add a comment explaining why we are us…

38749d9

…ing the builder pattern

already import configuration in this file - use short name

1a37b8f

asfgit closed this in c5eb1df Aug 4, 2016

[SPARK-16814][SQL] Fix deprecated parquet constructor usage #14419

[SPARK-16814][SQL] Fix deprecated parquet constructor usage #14419

Uh oh!

Conversation

holdenk commented Jul 30, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jul 30, 2016

Uh oh!

srowen commented Jul 30, 2016

Uh oh!

HyukjinKwon commented Jul 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Jul 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

holdenk commented Jul 30, 2016

Uh oh!

holdenk commented Jul 30, 2016

Uh oh!

HyukjinKwon commented Jul 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Aug 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 31, 2016

Uh oh!

holdenk commented Jul 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 3, 2016

Uh oh!

srowen commented Aug 4, 2016

Uh oh!

Uh oh!

HyukjinKwon commented Jul 30, 2016 •

edited

Loading

HyukjinKwon commented Jul 30, 2016 •

edited

Loading

HyukjinKwon commented Jul 30, 2016 •

edited

Loading

HyukjinKwon Aug 3, 2016 •

edited

Loading