----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4940/#review7587 -----------------------------------------------------------
Looks good overall. Some nitpicks and minor changes. Will try and run through it again. /trunk/src/java/org/apache/hcatalog/api/HCatClient.java <https://reviews.apache.org/r/4940/#comment16774> the long? /trunk/src/java/org/apache/hcatalog/api/HCatCreateDBDesc.java <https://reviews.apache.org/r/4940/#comment16780> nitpick: I like "toHiveDB" better like "toString", "toInt", etc. /trunk/src/java/org/apache/hcatalog/api/HCatCreateTableDesc.java <https://reviews.apache.org/r/4940/#comment16781> nitpick: indentation /trunk/src/java/org/apache/hcatalog/api/HCatCreateTableDesc.java <https://reviews.apache.org/r/4940/#comment16795> indentation is not aligned /trunk/src/java/org/apache/hcatalog/api/HCatCreateTableDesc.java <https://reviews.apache.org/r/4940/#comment16783> can numBuckets exist without buketCols being set? If this isn't the case maybe they should be combined into just one builder method? /trunk/src/java/org/apache/hcatalog/api/HCatHMSImpl.java <https://reviews.apache.org/r/4940/#comment16785> nitpick: HCatClientHMSImpl? /trunk/src/java/org/apache/hcatalog/api/HCatHMSImpl.java <https://reviews.apache.org/r/4940/#comment16786> nitpick: indentation /trunk/src/java/org/apache/hcatalog/api/HCatHMSImpl.java <https://reviews.apache.org/r/4940/#comment16787> should we be putting logic here? this should just be a thin wrapper. /trunk/src/java/org/apache/hcatalog/api/HCatHMSImpl.java <https://reviews.apache.org/r/4940/#comment16788> same with this logic. /trunk/src/java/org/apache/hcatalog/api/HCatHMSImpl.java <https://reviews.apache.org/r/4940/#comment16789> if we don't set it what will happen? will hms set it for us? /trunk/src/java/org/apache/hcatalog/api/HCatPartition.java <https://reviews.apache.org/r/4940/#comment16790> why is this always null? /trunk/src/java/org/apache/hcatalog/common/HCatUtil.java <https://reviews.apache.org/r/4940/#comment16775> It seems cleaner to encapsulate this logic in HCatClient.create()? /trunk/src/test/org/apache/hcatalog/api/TestHCatClient.java <https://reviews.apache.org/r/4940/#comment16793> I'd have the same statement execute before this but with ifNotExists set to false. This way you get to test that the previous table was dropped. /trunk/src/test/org/apache/hcatalog/api/TestHCatClient.java <https://reviews.apache.org/r/4940/#comment16794> should verify the schema as well - Francis On 2012-05-03 00:24:21, Vandana Ayyalasomayajula wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4940/ > ----------------------------------------------------------- > > (Updated 2012-05-03 00:24:21) > > > Review request for hcatalog, Francis Liu, David Capwell, and Rohini > Palaniswamy. > > > Summary > ------- > > HCatalog java APIs for DDL commands. > > > Diffs > ----- > > /trunk/src/java/org/apache/hcatalog/api/HCatAddPartitionDesc.java > PRE-CREATION > /trunk/src/java/org/apache/hcatalog/api/HCatClient.java PRE-CREATION > /trunk/src/java/org/apache/hcatalog/api/HCatCreateDBDesc.java PRE-CREATION > /trunk/src/java/org/apache/hcatalog/api/HCatCreateTableDesc.java > PRE-CREATION > /trunk/src/java/org/apache/hcatalog/api/HCatDatabase.java PRE-CREATION > /trunk/src/java/org/apache/hcatalog/api/HCatHMSImpl.java PRE-CREATION > /trunk/src/java/org/apache/hcatalog/api/HCatPartition.java PRE-CREATION > /trunk/src/java/org/apache/hcatalog/api/HCatTable.java PRE-CREATION > /trunk/src/java/org/apache/hcatalog/common/HCatConstants.java 1333272 > /trunk/src/java/org/apache/hcatalog/common/HCatUtil.java 1333272 > /trunk/src/test/org/apache/hcatalog/api/TestHCatClient.java PRE-CREATION > > Diff: https://reviews.apache.org/r/4940/diff > > > Testing > ------- > > Unit test passes. > > TODO: > > - cover more test cases. > - E2E tests. > > > Thanks, > > Vandana > >
