----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16951/#review32561 -----------------------------------------------------------
Hi, Sorry for the delay, I thought I'd published this review over the weekend, but reviewboard was unresponsive, and it looks like it didn't save. hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/FileOutputCommitterContainer.java <https://reviews.apache.org/r/16951/#comment61449> "_DYN" is already defined in FosterStorageHandler, needs to have one place where it's defined. I'm okay with it being defined here if the FosterStorageHandler constant is removed and references to that are changed to reference this. hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/FileOutputCommitterContainer.java <https://reviews.apache.org/r/16951/#comment61451> whitespace errors - git refers to a bunch of these through the patch when we try to apply, please correct for final patch upload. hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/FileOutputCommitterContainer.java <https://reviews.apache.org/r/16951/#comment61453> A bit about code readability - if we add a special case, then it makes sense to add the special case as an else, rather than as an if - that way, the default behaviour is visible first, and then the special case - please swap this around so that this is a if (!customDynamicLocationUsed) structure. hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/FileOutputCommitterContainer.java <https://reviews.apache.org/r/16951/#comment61452> This is now significant amount of code repetition from line 720-741 above, please see if we can refactor this into a separate method. hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/OutputJobInfo.java <https://reviews.apache.org/r/16951/#comment61455> This becomes the primary API point with this change, wherein, a user that is using HCatOutputFormat will generate an OutputJobInfo, and then call setCustomDynamicLocation on it. This is fine for M/R users of HCat, but is something that will wind up having to be implemented for each M/R user. It might have been better to define a constant in HCatConstants, say "hcat.dynamic.partitioning.custom.pattern", and to use that as a JobInfo parameter. That makes it easier for other tools to integrate with this feature. For example, with your patch, we still do not support the ability for the HCatStorer from pig to be able to write to custom dynamic partitions, while we do want to keep feature parity where possible between HCatOutputFormat and HCatStorer. In fact, as a design goal for HCat, we're trying to move away from letting(requiring) users explicitly muck around with OutputJobInfo and InputJobInfo, and stick to static calls to HCatInputFormat/HCatOutputFormat. I would like to see this call be something the HCatOutputFormat automatically calls if a jobConf parameter(as above) is set. That way, we can solve pig compatibility as well easily. - Sushanth Sowmyan On Jan. 16, 2014, 12:09 p.m., Satish Mittal wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16951/ > ----------------------------------------------------------- > > (Updated Jan. 16, 2014, 12:09 p.m.) > > > Review request for hive and Sushanth Sowmyan. > > > Repository: hive-git > > > Description > ------- > > - Attaching the patch that implements the functionality to support custom > location for external tables in dynamic partitioning. > > > Diffs > ----- > > > hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/FileOutputCommitterContainer.java > a5ae1be > > hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/FosterStorageHandler.java > 288b7a3 > > hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/HCatFileUtil.java > PRE-CREATION > > hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/OutputJobInfo.java > b63bdc2 > > hcatalog/core/src/test/java/org/apache/hive/hcatalog/mapreduce/HCatMapReduceTest.java > 77bdb9d > > hcatalog/core/src/test/java/org/apache/hive/hcatalog/mapreduce/TestHCatDynamicPartitioned.java > d8b69c2 > > hcatalog/core/src/test/java/org/apache/hive/hcatalog/mapreduce/TestHCatExternalDynamicPartitioned.java > 36c7945 > > Diff: https://reviews.apache.org/r/16951/diff/ > > > Testing > ------- > > - Added unit test. > - Tested the functionality through a sample MR program that uses > HCatOutputFormat interface configured with the new custom dynamic location. > > > Thanks, > > Satish Mittal > >
