PARQUET-1292 Adding constructors to ProtoParquetWriter with writeSpecsCompliant flag

[chawlakunal]

This PR adds constructors to ProtoParquetWriter with flags for specs compliant writer (based on #411). Now a specs compliant ProtoParquetWriter can be created as

new ProtoParquetWriter(file, protoMessageClass, true);

[BenoitHanotte]

I believe the default block size is 128MB by default (see https://github.com/apache/parquet-mr/blob/0d55abd05b0e5027c18e60d1ac3b22998dd00951/parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetWriter.java#L41

[BenoitHanotte]

Thanks @chawlakunal for the commit, it looks good to me. I believe there is a slight mistake in a comment (see my review above).
You may want to open a Jira ticket for this commit and add the ticket name in the message ;)

[chawlakunal]

@BenoitHanotte Happy to contribute!!1

I have created a JIRA for this but I am unable to assign it to myself.

[zivanfi]

@chawlakunal, I added you the list of contributors so that you can assign JIRA-s to yourself. I assigned this one to you to make sure it works.

[chawlakunal]

@BenoitHanotte Can this be approved and merged?

[lukasnalezenec]

Hi,
Thank you for your work.
I looked at it briefly, I will send you detailed opinion after weekend.
I am not sure if we need so many new constructors.
We should publish the Configuration parameter from superclass.

[chawlakunal]

Hi @lukasnalezenec
Waiting to hear from you.

[costimuraru]

+1 on finding a way to pass the writeSpecsCompliant flag directly to the ProtoParquetWriter and simplify things.

Right now we have to do something like:

Configuration conf = new Configuration();
ProtoWriteSupport.setWriteSpecsCompliant(conf, true);

ParquetWriter<MyMessage> writer 
   = new ParquetWriter<>(file, new ProtoWriteSupport<MyMessage>(cls), CompressionCodecName.GZIP, 256 * 1024 * 1024, 1 * 1024 * 1024,  1 * 1024 * 1024, true, false, ParquetWriter.DEFAULT_WRITER_VERSION, conf);

[chawlakunal]

I am still waiting for comments from @lukasnalezenec

[lukasnalezenec]

I think that problem is that there was no ProtoParquetWriter constructor with conf parameter.
I would recommend adding the conf parameter and not writeSpecsCompliant parameter.
AFAIK there is no elegant solution how we can accept writeSpecsCompliant parameter, set its value to configuration and pass it super constructor.
(see getConfigWithWriteSpecsCompliant())

I would take existing constructors and add those two new constructors:

ProtoParquetWriter(Path file, Class<? extends Message> protoMessage, Configuration conf);

ProtoParquetWriter(Path file, Class<? extends Message> protoMessage, ParquetFileWriter.Mode mode, CompressionCodecName compressionCodecName, int blockSize, int pageSize, int dictionaryPageSize, boolean enableDictionary, boolean validating, WriterVersion writerVersion, Configuration conf

We might also extend some other existing constructors with conf parameter. (I am not sure if it is necessary)

After this change we might turn on the parameter like this:

Configuration conf = new Configuration();
ProtoWriteSupport.setWriteSpecsCompliant(conf, true);

ParquetWriter<MyMessage> writer  = new ProtoParquetWriter<>(file, cls, conf);`

If you disagree with me, I'm open to another solution.

[costimuraru]

@lukasnalezenec From my perspective, it looks great!

[BenoitHanotte]

Hello @costimuraru @chawlakunal
As we might again be adding a new flag if #410 passes, I think we may want to follow @lukasnalezenec suggestion and try to have constructors that accept the conf object, which would allow to avoid duplicating all the existing constructors for each new flag.

[chawlakunal]

@BenoitHanotte I'll work on that sometime this week.

[costimuraru]

@chawlakunal any luck?

[georgehdd]

@chawlakunal Any news on this?