----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59020/#review180234 -----------------------------------------------------------
Fix it, then Ship it! The patch looks good Adam. Please just add the comments and remove the unused imports. hcatalog/hcatalog-pig-adapter/src/test/java/org/apache/hive/hcatalog/pig/TestHCatStorerMulti.java Lines 46-49 (original), 46-49 (patched) <https://reviews.apache.org/r/59020/#comment255324> There are a few unused imports. Remove them. hcatalog/hcatalog-pig-adapter/src/test/java/org/apache/hive/hcatalog/pig/TestParquetHCatStorer.java Lines 21-28 (original), 21-28 (patched) <https://reviews.apache.org/r/59020/#comment255323> There are a few unused imports. Remove them. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/MapredParquetOutputFormat.java Line 76 (original), 76 (patched) <https://reviews.apache.org/r/59020/#comment255325> Could you add a javadoc comment about what parameters are expected on the JobConf object? I see that this constructor won't work unless the PARQUET_HIVE_SCHEMA is set by the caller (HCat is doing in on the SpecialCases), and we have to leave that comment about the assumptions. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ParquetTableUtils.java Lines 24 (patched) <https://reviews.apache.org/r/59020/#comment255309> I like to avoid short words on method names. What bout just using isParquetProperty() instead? The class name already implies this is meant for parquet tables. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/ParquetRecordWriterWrapper.java Lines 87 (patched) <https://reviews.apache.org/r/59020/#comment255312> Same thing with the short words on method names. However, makes more sense to use getParquetProperties(), what do you think? Table properties are set through the Table, but jobConf can bring other parquet properties that were set in hive-site.xml, right? - Sergio Pena On May 5, 2017, 8:11 a.m., Adam Szita wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59020/ > ----------------------------------------------------------- > > (Updated May 5, 2017, 8:11 a.m.) > > > Review request for hive, Aihua Xu and Sergio Pena. > > > Bugs: HIVE-8838 > https://issues.apache.org/jira/browse/HIVE-8838 > > > Repository: hive-git > > > Description > ------- > > Adding support for HCatalog to write tables stored in Parquet format > > > Diffs > ----- > > > hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/FileRecordWriterContainer.java > b2abc5fbb3670893415354552239d67d072459ed > > hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/SpecialCases.java > 60af5c0bf397273fb820f0ee31e578745dbc200f > > hcatalog/hcatalog-pig-adapter/src/test/java/org/apache/hive/hcatalog/pig/TestHCatLoaderComplexSchema.java > 4c686fec596d39d41d458bc3ea2753877bd9df98 > > hcatalog/hcatalog-pig-adapter/src/test/java/org/apache/hive/hcatalog/pig/TestHCatLoaderEncryption.java > ad11eab1b7e67541b56e90e4a85ba37b41a4db92 > > hcatalog/hcatalog-pig-adapter/src/test/java/org/apache/hive/hcatalog/pig/TestHCatStorerMulti.java > 918332ddfda58306707d326f8668b2c223110a29 > > hcatalog/hcatalog-pig-adapter/src/test/java/org/apache/hive/hcatalog/pig/TestParquetHCatLoader.java > 6cd382145b55d6b85fc3366faeaba2aaef65ab04 > > hcatalog/hcatalog-pig-adapter/src/test/java/org/apache/hive/hcatalog/pig/TestParquetHCatStorer.java > 6dfdc04954dd0b110b1a7194e69468b5dc2f842e > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/MapredParquetOutputFormat.java > a7bb5eedbb99f3cea4601b9fce9a0ad3461567d0 > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ParquetTableUtils.java > b339cc4347eea143dca2f6d98f9aa7777afdc427 > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriteSupport.java > 71a78cf040667bf14b6c720373e4acd102da19f4 > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/ParquetRecordWriterWrapper.java > c021dafa480e65d7c0c19a5a85988464112468cb > > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestMapredParquetOutputFormat.java > ec85b5df0f95cbd45b87259346ae9c1e5aa604a4 > > > Diff: https://reviews.apache.org/r/59020/diff/1/ > > > Testing > ------- > > Tested on cluster, and re-enabled previously disabled tests in HCatalog (for > Parquet) that were failing (this adds ~40 tests to be run) > > > Thanks, > > Adam Szita > >
