Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Hive, mail # dev - Re: Review Request 11925: Hive-3159 Update AvroSerde to determine schema of new tables


Copy link to this message
-
Re: Review Request 11925: Hive-3159 Update AvroSerde to determine schema of new tables
Carl Steinbach 2014-01-11, 07:39

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