hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Xuefu Zhang" <xzh...@cloudera.com>
Subject Re: Review Request 17061: HIVE-5783 - Native Parquet Support in Hive
Date Mon, 03 Feb 2014 23:53:49 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message