----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25669/#review54028 -----------------------------------------------------------
Looks good to me! I have a few items below but otherwise it looks great. hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseSerDeParameters.java <https://reviews.apache.org/r/25669/#comment93911> can be re-written as: STRUCT_SERIALIZATION_TYPE.equals(serType) leaving the null check to String.equals. hbase-handler/src/java/org/apache/hadoop/hive/hbase/struct/DefaultHBaseValueFactory.java <https://reviews.apache.org/r/25669/#comment93912> Not related this this change, but would you mind fixing the spacing in this file? hbase-handler/src/java/org/apache/hadoop/hive/hbase/struct/HBaseStructValue.java <https://reviews.apache.org/r/25669/#comment93913> trim trailing ws in this file (shows up as red) hbase-handler/src/java/org/apache/hadoop/hive/hbase/struct/HBaseStructValue.java <https://reviews.apache.org/r/25669/#comment93914> these fields should either be private or protected hbase-handler/src/java/org/apache/hadoop/hive/hbase/struct/HBaseValueFactory.java <https://reviews.apache.org/r/25669/#comment93915> trim trailing ws hbase-handler/src/test/org/apache/hadoop/hive/hbase/HBaseTestStructSerializer.java <https://reviews.apache.org/r/25669/#comment93916> These fields should be private or protected hbase-handler/src/test/org/apache/hadoop/hive/hbase/HBaseTestStructSerializer.java <https://reviews.apache.org/r/25669/#comment93917> trim ws hbase-handler/src/test/org/apache/hadoop/hive/hbase/TestHBaseSerDe.java <https://reviews.apache.org/r/25669/#comment93923> let's trim some of these new lines between each stmt hbase-handler/src/test/org/apache/hadoop/hive/hbase/TestHBaseSerDe.java <https://reviews.apache.org/r/25669/#comment93920> Should be there be a flag to ensure you acutally found a LazyStruct ? hbase-handler/src/test/org/apache/hadoop/hive/hbase/TestHBaseSerDe.java <https://reviews.apache.org/r/25669/#comment93918> trim trailing ws also did you mean to log the output from SerDeUtils.getJSONString(row, soi);? L - Brock Noland On Sept. 15, 2014, 10:37 p.m., Swarnim Kulkarni wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25669/ > ----------------------------------------------------------- > > (Updated Sept. 15, 2014, 10:37 p.m.) > > > Review request for hive. > > > Bugs: HIVE-6148 > https://issues.apache.org/jira/browse/HIVE-6148 > > > Repository: hive-git > > > Description > ------- > > We should add support to be able to query arbitrary structs stored in HBase. > > > Diffs > ----- > > hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseSerDe.java ca2f40e > hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseSerDeHelper.java > 25a9cfc > > hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseSerDeParameters.java > 8878eb5 > hbase-handler/src/java/org/apache/hadoop/hive/hbase/LazyHBaseRow.java > 3e8b8fd > > hbase-handler/src/java/org/apache/hadoop/hive/hbase/struct/AvroHBaseValueFactory.java > c341c0a > > hbase-handler/src/java/org/apache/hadoop/hive/hbase/struct/DefaultHBaseValueFactory.java > ac2cb57 > > hbase-handler/src/java/org/apache/hadoop/hive/hbase/struct/HBaseStructValue.java > PRE-CREATION > > hbase-handler/src/java/org/apache/hadoop/hive/hbase/struct/HBaseValueFactory.java > 8722af0 > > hbase-handler/src/java/org/apache/hadoop/hive/hbase/struct/StructHBaseValueFactory.java > PRE-CREATION > > hbase-handler/src/test/org/apache/hadoop/hive/hbase/HBaseTestStructSerializer.java > PRE-CREATION > hbase-handler/src/test/org/apache/hadoop/hive/hbase/TestHBaseSerDe.java > 241818a > > Diff: https://reviews.apache.org/r/25669/diff/ > > > Testing > ------- > > Additional unit tests added. > > > Thanks, > > Swarnim Kulkarni > >
