----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28372/#review63424 -----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java <https://reviews.apache.org/r/28372/#comment105680> What does this change do? ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/ColInfoFromParquetFile.java <https://reviews.apache.org/r/28372/#comment105672> I don't think ColInfoFromParquetFile is a great name for this class. What you've implemented here is a schema converter, like HiveSchemaConverter but from MessageType to TypeInfo rather than the other way around. I think this should either go in HiveSchemaConverter or a new class, like ParquetToHiveSchemaConverter. This update would also help clarify the methods exposed. I think that the primary method this should expose is: StructType convert(GroupType parquetSchema) File reading should be done elsewhere if it is necessary. Also, I think this should return a StructType instead of a Pair with Strings. That would be a lot cleaner and would avoid the custom type string building immediately followed by parsing those type strings. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ParquetHiveSerDe.java <https://reviews.apache.org/r/28372/#comment105675> Could you please undo the import moves? I like to avoid non-functional changes. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ParquetHiveSerDe.java <https://reviews.apache.org/r/28372/#comment105676> Is it possible to avoid setting a file property? There are lots of cases where a file in the dataset would be removed so this is a brittle method of configuring the table. Ideally, we would check for the LIST_COLUMN_TYPES property and if we don't find it, conver the schema of the first file that we find. ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ParquetHiveSerDe.java <https://reviews.apache.org/r/28372/#comment105678> This non-functional change is fine with me because it fixes the style in a method you're editing. But if you add a newline after this conditional, then you should also add one after line 149. - Ryan Blue On Nov. 26, 2014, 5:08 p.m., Ashish Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28372/ > ----------------------------------------------------------- > > (Updated Nov. 26, 2014, 5:08 p.m.) > > > Review request for hive. > > > Bugs: HIVE-8950 > https://issues.apache.org/jira/browse/HIVE-8950 > > > Repository: hive-git > > > Description > ------- > > HIVE-8950: Add support in ParquetHiveSerde to create table schema from a > parquet file > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java > fafd78e63e9b41c9fdb0e017b567dc719d151784 > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/ColInfoFromParquetFile.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ParquetHiveSerDe.java > 4effe736fcf9d3715f03eed9885c299a7aa040dd > > ql/src/test/queries/clientpositive/parquet_array_of_multi_field_struct_gen_schema.q > PRE-CREATION > > ql/src/test/queries/clientpositive/parquet_array_of_optional_elements_gen_schema.q > PRE-CREATION > > ql/src/test/queries/clientpositive/parquet_array_of_required_elements_gen_schema.q > PRE-CREATION > > ql/src/test/queries/clientpositive/parquet_array_of_single_field_struct_gen_schema.q > PRE-CREATION > ql/src/test/queries/clientpositive/parquet_array_of_structs_gen_schema.q > PRE-CREATION > > ql/src/test/queries/clientpositive/parquet_array_of_unannotated_groups_gen_schema.q > PRE-CREATION > > ql/src/test/queries/clientpositive/parquet_array_of_unannotated_primitives_gen_schema.q > PRE-CREATION > > ql/src/test/queries/clientpositive/parquet_avro_array_of_primitives_gen_schema.q > PRE-CREATION > > ql/src/test/queries/clientpositive/parquet_avro_array_of_single_field_struct_gen_schema.q > PRE-CREATION > ql/src/test/queries/clientpositive/parquet_decimal_gen_schema.q > PRE-CREATION > > ql/src/test/queries/clientpositive/parquet_thrift_array_of_primitives_gen_schema.q > PRE-CREATION > > ql/src/test/queries/clientpositive/parquet_thrift_array_of_single_field_struct_gen_schema.q > PRE-CREATION > > ql/src/test/results/clientpositive/parquet_array_of_multi_field_struct_gen_schema.q.out > PRE-CREATION > > ql/src/test/results/clientpositive/parquet_array_of_optional_elements_gen_schema.q.out > PRE-CREATION > > ql/src/test/results/clientpositive/parquet_array_of_required_elements_gen_schema.q.out > PRE-CREATION > > ql/src/test/results/clientpositive/parquet_array_of_single_field_struct_gen_schema.q.out > PRE-CREATION > > ql/src/test/results/clientpositive/parquet_array_of_structs_gen_schema.q.out > PRE-CREATION > > ql/src/test/results/clientpositive/parquet_array_of_unannotated_groups_gen_schema.q.out > PRE-CREATION > > ql/src/test/results/clientpositive/parquet_array_of_unannotated_primitives_gen_schema.q.out > PRE-CREATION > > ql/src/test/results/clientpositive/parquet_avro_array_of_primitives_gen_schema.q.out > PRE-CREATION > > ql/src/test/results/clientpositive/parquet_avro_array_of_single_field_struct_gen_schema.q.out > PRE-CREATION > ql/src/test/results/clientpositive/parquet_decimal_gen_schema.q.out > PRE-CREATION > > ql/src/test/results/clientpositive/parquet_thrift_array_of_primitives_gen_schema.q.out > PRE-CREATION > > ql/src/test/results/clientpositive/parquet_thrift_array_of_single_field_struct_gen_schema.q.out > PRE-CREATION > > Diff: https://reviews.apache.org/r/28372/diff/ > > > Testing > ------- > > Tested by adding appropriate qTests. > > > Thanks, > > Ashish Singh > >
