----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20096/#review40245 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/plan/PartitionDesc.java <https://reviews.apache.org/r/20096/#comment73193> private static final variable names should be ALL_CAPS_WITH_UNDERSCORES (see variables on preceding lines). ql/src/java/org/apache/hadoop/hive/ql/plan/PartitionDesc.java <https://reviews.apache.org/r/20096/#comment73194> Formatting and whitespace cleanup should generally be reserved for patches specifically devoted to that task. While I sympathize with the urge to clean things up it makes backporting and merging patches a lot harder. If your IDE is automatically doing this you need to disable this behavior. ql/src/java/org/apache/hadoop/hive/ql/plan/PartitionDesc.java <https://reviews.apache.org/r/20096/#comment73198> I think it would be good to explain the motivation for this change in the comment. ql/src/java/org/apache/hadoop/hive/ql/plan/PartitionDesc.java <https://reviews.apache.org/r/20096/#comment73199> I think this would be a bit cleaner if lines 173 and 174 were left unchanged and line 181 was updated to iterate over tableDesc.getProperties(). ql/src/test/queries/clientpositive/avro_partitioned.q <https://reviews.apache.org/r/20096/#comment73195> Good attention to detail! ql/src/test/queries/clientpositive/avro_partitioned.q <https://reviews.apache.org/r/20096/#comment73196> May want to add "... even if it has an old schema relative to the current table level schema". serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerdeUtils.java <https://reviews.apache.org/r/20096/#comment73200> We should avoid defining this string token in two locations. I think it makes sense to refer to the one in PartitionDesc. serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerdeUtils.java <https://reviews.apache.org/r/20096/#comment73201> I think it's a little confusing that useTablePropertiesIfAvailable mutates the contents of the properties object, which is then read on the next line. I think this code will be easier to understand if useTablePropertiesIfAvailable is eliminated and the code is moved into an if/else if/else block in determineSchemaOrThrowException(). serde/src/test/org/apache/hadoop/hive/serde2/avro/TestAvroSerdeUtils.java <https://reviews.apache.org/r/20096/#comment73202> A comment explaining what you're testing would be nice. - Carl Steinbach On April 7, 2014, 7:18 p.m., Anthony Hsu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20096/ > ----------------------------------------------------------- > > (Updated April 7, 2014, 7:18 p.m.) > > > Review request for hive. > > > Repository: hive-git > > > Description > ------- > > The problem occurs when you store the "avro.schema.(literal|url)" in the > SERDEPROPERTIES instead of the TBLPROPERTIES, add a partition, change the > table's schema, and then try reading from the old partition. > > I fixed this problem by passing the table properties to the partition with a > "table." prefix, and changing the Avro SerDe to always use the table > properties when available. > > > Diffs > ----- > > ql/src/java/org/apache/hadoop/hive/ql/plan/PartitionDesc.java 43cef5c > ql/src/test/queries/clientpositive/avro_partitioned.q 068a13c > ql/src/test/results/clientpositive/avro_partitioned.q.out 352ec0d > serde/src/java/org/apache/hadoop/hive/serde2/avro/AvroSerdeUtils.java > 9d58d13 > serde/src/test/org/apache/hadoop/hive/serde2/avro/TestAvroSerdeUtils.java > 67d5570 > > Diff: https://reviews.apache.org/r/20096/diff/ > > > Testing > ------- > > Added test cases > > > Thanks, > > Anthony Hsu > >
