----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30750/#review71603 -----------------------------------------------------------
Looks good! Sergio. I only have one question. In the 4 inspectors, I think we don't need to check (array length == 0), this may miss the empty map/list case. The original code does this check, but it is for container. What do you think? I left detailed comments for 1 inspector below. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/AbstractParquetMapInspector.java <https://reviews.apache.org/r/30750/#comment117426> In original wrapper, it has to check null and length to ensure there is an ArrayWritable of Map. Then get it. As we removed the wrapper, and get the mapArray already, maybe we just need to check null here. If length is 0, I think we should return an empty map instead of null. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/AbstractParquetMapInspector.java <https://reviews.apache.org/r/30750/#comment117427> how about just check null? Length being 0 might be a normal case. - Dong Chen On Feb. 8, 2015, 6:13 a.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30750/ > ----------------------------------------------------------- > > (Updated Feb. 8, 2015, 6:13 a.m.) > > > Review request for hive, Ryan Blue, cheng xu, and Dong Chen. > > > Bugs: HIVE-9605 > https://issues.apache.org/jira/browse/HIVE-9605 > > > Repository: hive-git > > > Description > ------- > > Remove wrapper object from parquet nested types (map/array) > > > Diffs > ----- > > > itests/hive-jmh/src/main/java/org/apache/hive/benchmark/storage/ColumnarStorageBench.java > d335716be3d286a1b9221dcbd8ccd799f4c6dc66 > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/HiveCollectionConverter.java > 6621a8768953a9bef54e7a144ae045abcc32f458 > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/HiveGroupConverter.java > 4809f9b5882ae409159b422c08c665aa24f796d8 > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/Repeated.java > fdea782167d63593f6cbde5e7154d771761757f7 > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/AbstractParquetMapInspector.java > 62c61fc7502f24e6a032076f384b5a946c1cc9a6 > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/DeepParquetHiveMapInspector.java > d38c64192e01371c0c98b339113348d2e52cedc3 > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ParquetHiveArrayInspector.java > 53ca31d0b516c4a941e048e98e7f8f763752c436 > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/StandardParquetHiveMapInspector.java > 5aa14482899fed5711b40c5554b056d07818afb5 > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestMapStructures.java > ca4805082fd717d15ed41ca15a730e19da267c8a > > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestAbstractParquetMapInspector.java > ef05150494027ddd70790dcf26b772ebc4cd2b8b > > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestDeepParquetHiveMapInspector.java > 8646ff4d3413d7d642e2559e1a485d77472b156a > > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestParquetHiveArrayInspector.java > f3a24af2e5f4eeb24e1e286ada19fc9592daacb6 > > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestStandardParquetHiveMapInspector.java > 278419f73b311322dcf3c70abb340bf63d8a4337 > > Diff: https://reviews.apache.org/r/30750/diff/ > > > Testing > ------- > > > Thanks, > > Sergio Pena > >
