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