----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3784/#review4941 -----------------------------------------------------------
trunk/src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/3784/#comment10857> not related do this patch. Didn't know the outputSchema is not in inputJobInfo. We should file a jira to put it in for consistency. trunk/src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/3784/#comment10859> since jobConf is a deep copy of jobContext.getConfiguration() you should be consistent here and only use jobConf. In case one of these methods actually modifies the configuration then you know all of it is in one place. And update the jobContext with all the changes before this method ends. trunk/src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/3784/#comment10866> We should sync up on how we do this. Take a look at my patch and HCatUtil.getStorageHandler(). trunk/src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/3784/#comment10874> Available in my patch use HCatUtil.getStorageHandler() instead trunk/src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/3784/#comment10863> Will jobProperties really have nothing in it? maybe you should append to the the contents instead? trunk/src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/3784/#comment10864> IOException would be better. Please add a message as well. trunk/src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/3784/#comment10869> should be moved into default/foster storageHandler logic. trunk/src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/3784/#comment10870> what happens if storageHandler == null? You should encapsulate if,of,serde into a "FosterStorageHandler" so you only have to take care of one code path. trunk/src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/3784/#comment10871> you don't need an explicit cast. trunk/src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/3784/#comment10872> For now use HCatStorageHandler since it contains some methods that are missing and will be added to HiveStorageHandler trunk/src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/3784/#comment10873> similar code as the previous...same comments. In HCatOutputFormat, I moved code such as this into HCatUtil.configureOutputJobProperties() maybe you should do something similar? trunk/src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/3784/#comment10875> I would just do: JobConf jobConf = new JobConf(jobContext.getConfiguration) One less variable to worry about. trunk/src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/3784/#comment10865> Since this is file based code. this logic should go into the Foster/Default StoragaHandler.configureInputJobProperties() trunk/src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/3784/#comment10876> you don't need the 2nd condition trunk/src/java/org/apache/hcatalog/mapreduce/HCatRecordReader.java <https://reviews.apache.org/r/3784/#comment10877> you can probably remove this already? - Francis On 2012-02-08 01:01:56, Alan Gates wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3784/ > ----------------------------------------------------------- > > (Updated 2012-02-08 01:01:56) > > > Review request for hcatalog, Sushanth Sowmyan and Francis Liu. > > > Summary > ------- > > See HCATALOG-237 for details and design notes. > > > This addresses bug HCATALOG-239. > https://issues.apache.org/jira/browse/HCATALOG-239 > > > Diffs > ----- > > trunk/src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java > 1241719 > trunk/src/java/org/apache/hcatalog/mapreduce/HCatEximInputFormat.java > 1241719 > trunk/src/java/org/apache/hcatalog/mapreduce/HCatInputFormat.java 1241719 > trunk/src/java/org/apache/hcatalog/mapreduce/HCatRecordReader.java 1241719 > trunk/src/java/org/apache/hcatalog/mapreduce/HCatSplit.java 1241719 > trunk/src/java/org/apache/hcatalog/mapreduce/InitializeInput.java 1241719 > trunk/src/java/org/apache/hcatalog/mapreduce/PartInfo.java 1241719 > > Diff: https://reviews.apache.org/r/3784/diff > > > Testing > ------- > > > Thanks, > > Alan > >
