----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17061/#review33473 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/io/parquet/MapredParquetInputFormat.java <https://reviews.apache.org/r/17061/#comment62912> This doesn't seem being used anywhere. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/MapredParquetOutputFormat.java <https://reviews.apache.org/r/17061/#comment62911> This doesn't seem necessary to be public. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/MapredParquetOutputFormat.java <https://reviews.apache.org/r/17061/#comment62935> If doubt exists, it's probably better to address it. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/ProjectionPusher.java <https://reviews.apache.org/r/17061/#comment62937> Please remove if not used. Make it private otherwise. The same applies to all code. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/ProjectionPusher.java <https://reviews.apache.org/r/17061/#comment62943> It's better to not to hard code those string consts here. They are probably defined somewhere. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/ProjectionPusher.java <https://reviews.apache.org/r/17061/#comment62951> I don't understand what the conversion is about: string -> path -> uri -> path -> string? ql/src/java/org/apache/hadoop/hive/ql/io/parquet/ProjectionPusher.java <https://reviews.apache.org/r/17061/#comment62953> Either put comments or log msg. Commented out code isn't comment. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/ProjectionPusher.java <https://reviews.apache.org/r/17061/#comment62954> Same as above. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/ProjectionPusher.java <https://reviews.apache.org/r/17061/#comment62957> Could you put more comments about what sort of refactoring? Can we log a JIRA for it also? ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/ArrayWritableGroupConverter.java <https://reviews.apache.org/r/17061/#comment62991> The if ... else ... here doesn't seem terribly different. Please refeactor the code. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/DataWritableGroupConverter.java <https://reviews.apache.org/r/17061/#comment62996> It seems that for (int i = 0; i < selectFieldCount; i++) is better for this loop. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/DataWritableGroupConverter.java <https://reviews.apache.org/r/17061/#comment62997> Please remove the extra blank lines, which are not necessary. The same applies to all code changes. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/ETypeConverter.java <https://reviews.apache.org/r/17061/#comment62999> Decimal treated as double? I don't think that's acceptable. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/ETypeConverter.java <https://reviews.apache.org/r/17061/#comment63000> Please change to public static ... ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/HiveSchemaConverter.java <https://reviews.apache.org/r/17061/#comment63003> Please change to "public static", across all code changes. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/HiveSchemaConverter.java <https://reviews.apache.org/r/17061/#comment63007> Please wrap long lines. Applicable to all code changes. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/DataWritableReadSupport.java <https://reviews.apache.org/r/17061/#comment63011> 1. private static? 2. use string constant instead of literal ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/DataWritableReadSupport.java <https://reviews.apache.org/r/17061/#comment63021> These string constant should be defined globally and referred here (and anywhere else). ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/DataWritableReadSupport.java <https://reviews.apache.org/r/17061/#comment63020> If not supposed to be call, it's better to throw an exception. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/DataWritableReadSupport.java <https://reviews.apache.org/r/17061/#comment63022> Same as above. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/DataWritableReadSupport.java <https://reviews.apache.org/r/17061/#comment63023> long too long ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/ParquetRecordReaderWrapper.java <https://reviews.apache.org/r/17061/#comment63024> long lines. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/AbstractParquetMapInspector.java <https://reviews.apache.org/r/17061/#comment63030> I don't understand why Hive would inspect an inspected result. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ArrayWritableObjectInspector.java <https://reviews.apache.org/r/17061/#comment63029> I don't understand why Hive would inspect an inspected result. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/DeepParquetHiveMapInspector.java <https://reviews.apache.org/r/17061/#comment63035> Again, why? ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ParquetHiveArrayInspector.java <https://reviews.apache.org/r/17061/#comment63038> Same here too. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ParquetHiveSerDe.java <https://reviews.apache.org/r/17061/#comment63039> final? ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ParquetHiveSerDe.java <https://reviews.apache.org/r/17061/#comment63040> Please remove extra blank lines. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ParquetHiveSerDe.java <https://reviews.apache.org/r/17061/#comment63041> String constants? ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ParquetHiveSerDe.java <https://reviews.apache.org/r/17061/#comment63043> for(int i = 0; i < fields.size(); i++) seems more appropriate. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ParquetHiveSerDe.java <https://reviews.apache.org/r/17061/#comment63044> Please remove extrac blank lines. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/StandardParquetHiveMapInspector.java <https://reviews.apache.org/r/17061/#comment63045> Again, why? We don't code against wrong assumption. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/writable/BinaryWritable.java <https://reviews.apache.org/r/17061/#comment63046> Do we have a followup JIRA? ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriteSupport.java <https://reviews.apache.org/r/17061/#comment63048> Move this to the top? ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java <https://reviews.apache.org/r/17061/#comment63049> remove extrac blank lines. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java <https://reviews.apache.org/r/17061/#comment63050> remove extrac blank lines. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java <https://reviews.apache.org/r/17061/#comment63052> We need to be consistent for decimal. I see code dealing with it, and also code that disallows it. ql/src/java/parquet/hive/DeprecatedParquetInputFormat.java <https://reviews.apache.org/r/17061/#comment63054> I don't quite get how this provides backward compatibility. Please explain. ql/src/java/parquet/hive/DeprecatedParquetOutputFormat.java <https://reviews.apache.org/r/17061/#comment63055> Same as above. - Xuefu Zhang On Jan. 30, 2014, 2:48 p.m., Brock Noland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17061/ > ----------------------------------------------------------- > > (Updated Jan. 30, 2014, 2:48 p.m.) > > > Review request for hive. > > > Bugs: HIVE-5783 > https://issues.apache.org/jira/browse/HIVE-5783 > > > Repository: hive-git > > > Description > ------- > > Adds native parquet support hive > > > Diffs > ----- > > data/files/parquet_create.txt PRE-CREATION > data/files/parquet_partitioned.txt PRE-CREATION > pom.xml 41f5337 > ql/pom.xml 7087a4c > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/MapredParquetInputFormat.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/MapredParquetOutputFormat.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/ProjectionPusher.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/ArrayWritableGroupConverter.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/DataWritableGroupConverter.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/DataWritableRecordConverter.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/ETypeConverter.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/HiveGroupConverter.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/HiveSchemaConverter.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/DataWritableReadSupport.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/ParquetRecordReaderWrapper.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/AbstractParquetMapInspector.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ArrayWritableObjectInspector.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/DeepParquetHiveMapInspector.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ParquetHiveArrayInspector.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ParquetHiveSerDe.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/StandardParquetHiveMapInspector.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/primitive/ParquetByteInspector.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/primitive/ParquetPrimitiveInspectorFactory.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/primitive/ParquetShortInspector.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/primitive/ParquetStringInspector.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/writable/BigDecimalWritable.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/writable/BinaryWritable.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriteSupport.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/ParquetRecordWriterWrapper.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java > 13d0a56 > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g f83c15d > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 010e04f > ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g 538b2b0 > ql/src/java/parquet/hive/DeprecatedParquetInputFormat.java PRE-CREATION > ql/src/java/parquet/hive/DeprecatedParquetOutputFormat.java PRE-CREATION > ql/src/java/parquet/hive/MapredParquetInputFormat.java PRE-CREATION > ql/src/java/parquet/hive/MapredParquetOutputFormat.java PRE-CREATION > ql/src/java/parquet/hive/serde/ParquetHiveSerDe.java PRE-CREATION > > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestHiveSchemaConverter.java > PRE-CREATION > > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestMapredParquetInputFormat.java > PRE-CREATION > > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestMapredParquetOutputFormat.java > PRE-CREATION > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestParquetSerDe.java > PRE-CREATION > > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestAbstractParquetMapInspector.java > PRE-CREATION > > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestDeepParquetHiveMapInspector.java > PRE-CREATION > > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestParquetHiveArrayInspector.java > PRE-CREATION > > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestStandardParquetHiveMapInspector.java > PRE-CREATION > ql/src/test/queries/clientpositive/parquet_create.q PRE-CREATION > ql/src/test/queries/clientpositive/parquet_partitioned.q PRE-CREATION > ql/src/test/results/clientpositive/parquet_create.q.out PRE-CREATION > ql/src/test/results/clientpositive/parquet_partitioned.q.out PRE-CREATION > > Diff: https://reviews.apache.org/r/17061/diff/ > > > Testing > ------- > > > Thanks, > > Brock Noland > >
