hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Carl Steinbach" <...@apache.org>
Subject Re: Review Request 11925: Hive-3159 Update AvroSerde to determine schema of new tables
Date Sat, 11 Jan 2014 07:39:46 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11925/#review31583
-----------------------------------------------------------



ql/src/test/queries/clientpositive/avro_create_as_select.q
<https://reviews.apache.org/r/11925/#comment60098>

    Please maintain reasonable limits on line length (80 or 100 characters works for me).



ql/src/test/queries/clientpositive/avro_create_as_select.q
<https://reviews.apache.org/r/11925/#comment60099>

    Please put each clause on its own line:
    CREATE TABLE xxx
    ROW FORMAT SERDE '....'
    STORED AS 
      INPUTFORMAT '....'
      OUTPUTFORMAT '....'
    AS
      SELECT ......
    



ql/src/test/queries/clientpositive/avro_create_as_select2.q
<https://reviews.apache.org/r/11925/#comment60100>

    Line length



ql/src/test/queries/clientpositive/avro_no_schema_test.q
<https://reviews.apache.org/r/11925/#comment60101>

    Line length.



serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerdeUtils.java
<https://reviews.apache.org/r/11925/#comment60102>

    Formatting



serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerdeUtils.java
<https://reviews.apache.org/r/11925/#comment60103>

    Formatting



serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerdeUtils.java
<https://reviews.apache.org/r/11925/#comment60104>

    Formatting



serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerdeUtils.java
<https://reviews.apache.org/r/11925/#comment60105>

    s/Defintion/Definition/



serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerdeUtils.java
<https://reviews.apache.org/r/11925/#comment60106>

    Please throw the exception here instead of returning null. This method should never return
null.



serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerdeUtils.java
<https://reviews.apache.org/r/11925/#comment60107>

    What's the significance of "org.apache.hive.auto_gen_schema"? I can't find any other references
to this namespace in the code.



serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerdeUtils.java
<https://reviews.apache.org/r/11925/#comment60115>

    table or partition properties, or both?



serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerdeUtils.java
<https://reviews.apache.org/r/11925/#comment60108>

    Formatting



serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java
<https://reviews.apache.org/r/11925/#comment60109>

    Missing ASF license header.



serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java
<https://reviews.apache.org/r/11925/#comment60110>

    To me "TypeInfoToSchema" sounds like the name of method, not a class. Please either change
the name to AvroSchemaUtils or merge this code into AvroSerdeUtils.
    
    Also, there is an existing Hive class named "Schema" (org.apache.hadoop.hive.metastore.api.Schema).
In order to avoid confusion please qualify "schema" with "avro" in the method names, e.g.
generateAvroSchema, generateAvroUnionSchema, etc...



serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java
<https://reviews.apache.org/r/11925/#comment60118>

    map<string, any-type>



serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java
<https://reviews.apache.org/r/11925/#comment60113>

    Why Hashtable instead of HashMap?



serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java
<https://reviews.apache.org/r/11925/#comment60111>

    People who don't already know how Avro encodes nullable values will find this method easier
to understand if the parameter name "wrapWithUnion" is changed to "isNullable".



serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java
<https://reviews.apache.org/r/11925/#comment60116>

    Instead of adding assertions to each of these methods I think it would be cleaner to specify
the expected type in the parameter list and force the caller to do the cast before invoking
the method.



serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java
<https://reviews.apache.org/r/11925/#comment60114>

    The code snippet "tag + "_" + tInfo.getCategory().name()" gets repeated a lot. I think
this should be moved to a helper method.



serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java
<https://reviews.apache.org/r/11925/#comment60112>

    Is it possible to change the LHS to List?



serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java
<https://reviews.apache.org/r/11925/#comment60117>

    Formatting.



serde/src/test/org/apache/hadoop/hive/serde2/avro/TestTypeInfoToSchema.java
<https://reviews.apache.org/r/11925/#comment60096>

    Missing ASF license header.



serde/src/test/org/apache/hadoop/hive/serde2/avro/TestTypeInfoToSchema.java
<https://reviews.apache.org/r/11925/#comment60097>

    Formatting.


- Carl Steinbach


On Jan. 10, 2014, 7:58 p.m., Mohammad Islam wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11925/
> -----------------------------------------------------------
> 
> (Updated Jan. 10, 2014, 7:58 p.m.)
> 
> 
> Review request for hive, Ashutosh Chauhan and Jakob Homan.
> 
> 
> Bugs: HIVE-3159
>     https://issues.apache.org/jira/browse/HIVE-3159
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> Problem:
> Hive doesn't support to create a Avro-based table using HQL create table command. It
currently requires to specify Avro schema literal or schema file name.
> For multiple cases, it is very inconvenient for user.
> Some of the un-supported use cases:
> 1. Create table ... <Avro-SERDE etc.> as SELECT ... from <NON-AVRO FILE>
> 2. Create table ... <Avro-SERDE etc.> as SELECT from <AVRO TABLE>
> 3. Create  table  without specifying Avro schema.
> 
> 
> Diffs
> -----
> 
>   ql/src/test/queries/clientpositive/avro_create_as_select.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/avro_create_as_select2.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/avro_no_schema_test.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/avro_without_schema.q PRE-CREATION 
>   ql/src/test/results/clientpositive/avro_create_as_select.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/avro_create_as_select2.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/avro_no_schema_test.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/avro_without_schema.q.out PRE-CREATION 
>   serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerdeUtils.java 9d58d13 
>   serde/src/java/org/apache/hadoop/hive/serde2/avro/TypeInfoToSchema.java PRE-CREATION

>   serde/src/test/org/apache/hadoop/hive/serde2/avro/TestAvroSerdeUtils.java 67d5570 
>   serde/src/test/org/apache/hadoop/hive/serde2/avro/TestTypeInfoToSchema.java PRE-CREATION

> 
> Diff: https://reviews.apache.org/r/11925/diff/
> 
> 
> Testing
> -------
> 
> Wrote a new java Test class for a new Java class. Added a new test case into existing
java test class. In addition, there are 4 .q file for testing multiple use-cases.
> 
> 
> Thanks,
> 
> Mohammad Islam
> 
>


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