ARROW-215: Support other integer types in Parquet I/O

[xhochy]

Code Review on Gerrit: https://gerrit.cloudera.org/#/c/3481/

[wesm]

We should look at whether using the Parquet 2.0 logical types will cause problems in other readers. e.g. Impala doesn't have these ConvertedType enums

https://github.com/cloudera/Impala/blob/cdh5-trunk/common/thrift/parquet.thrift

@julienledem can you advise us here?

What other systems do is deal with converting Parquet's physical INT32 to smaller types through a layering of additional metadata -- for example, if a Hive or Impala table has SMALLINT or TINYINT type, then the int32 values are materilized into int16 or int8 slots

[wesm]

What we might need to do is always return INT32 but provide an API where the user can request a particular column in the file to be casted to some other integer type.

[xhochy]

So this probably needs a Parquet 1/2 switch somewhere in the WriterProperties?

[wesm]

Yeah, I believe so. The problem is, at the moment Parquet 2 is not "in production". Even parquet-mr is still writing Parquet 1 files by default, so we should also default to parquet 1.x behavior for the time being

[wesm]

Didn't see any major issues w/ the code -- main thing we have to figure out is the Parquet 2.0 metadata problem -- if using the new ConvertedType values will cause issues, we may have to live with all integers coming back as either INT32 or INT64 on the read path -- we could add a setting that force integers to be casted to a primitive type. Either way the code you've got here for performing the conversion will be useful, there may just be some refactoring to do.

[xhochy]

Waiting for apache/parquet-cpp#119 to implement writer code whose behaviour depends on the Parquet version.

[xhochy]

Actually looking at the code again, this patch is working irrelevant for which Parquet version we are writing. The version handling has to be done in cpp/src/arrow/parquet/schema.cc as only the supported logical types are different between Parquet 2 and Parquet 1, the physical types are the same. Thus the write and read path are the same, only the metadata (and the tests depending on that) are changing.

[xhochy]

@wesm Is the PR ok for you or do we wait on other input?

I would proceed in a separate PR to implement the metadata switch between Parquet 1 and Parquet 2 in cpp/src/arrow/parquet/schema.cc.

Also when this PR is merged, I will directly open a new one for String I/O (that one is done but depends on this and I do not want to add a huge chunk of code to a nearly finished PR).

[wesm]

I'll review again shortly -- either way Parquet 1 must be the default as Parquet 2 is not yet widespread (or even fully specified, as far as I know)

[xhochy]

Made #93 which should go in first then to have the Parquet 1 vs 2 switch I would then use here to test the different type roundtrips depending on the Parquet version.

[wesm]

Sorry about the delay, I will look to merge this tomorrow morning

[xhochy]

Also pushed now the string support as this would otherwise involve quite some git magic to keep them in sync. So I can incorporate the review comments directly in this single PR:

[wesm]

Uwe -- would you mind pushing this review to gerrit? You can adapt the Kudu instructions

http://kudu.apache.org/docs/contributing.html#_initial_setup_for_gerrit

(remember to replace instances of "kudu" with "arrow" in the commands)

You will need to squash your commits into a single patch. Thanks

[xhochy]

@wesm done: https://gerrit.cloudera.org/#/c/3481/

[wesm]

Thanks. If you trigger a build then things should work now (per PARQUET-639). I'm working on the review, should finish tomorrow hopefully

[xhochy]

Yep, Travis is finally happy.

[xhochy]

Depends on apache/parquet-cpp#128

[xhochy]

Travis still fails as the travis job for parquet-cpp/OSX had a hiccup and we only have the newest parquet-cpp version for Linux.

[wesm]

+1 -- we conducted the code review on gerrit https://gerrit.cloudera.org/#/c/3481.