----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7043/#review11428 -----------------------------------------------------------
Overall this looks good Feng, thanks for diving into external table write support. Most of the comments below are documentation/polish rather than major changes. The only part of this change that's a little weird is requiring a wrapper. However, the issue there is changing the HCatStorer interface which is a separate issue, and if we choose to change that interface in the future this code is not affected. So I think its appropriate to treat them as separate issues. hcatalog-pig-adapter/src/test/java/org/apache/hcatalog/pig/HCatStorerWrapper.java <https://reviews.apache.org/r/7043/#comment24470> Please include a javadoc about this class as it could be quite confusing to someone seeing this the first time. I believe the reason this class exists is because we don't want to change the HCatStorer interface to add a new parameter, so this class is added for testing. If we do change the HCatStorer interface I really think we should consider using a single string and using an option parser, which gives us a lot of flexibility in the future, and would be the last time the interface needs to change. hcatalog-pig-adapter/src/test/java/org/apache/hcatalog/pig/TestHCatStorerWrapper.java <https://reviews.apache.org/r/7043/#comment24465> Please include a description of what this tests. hcatalog-pig-adapter/src/test/java/org/apache/hcatalog/pig/TestHCatStorerWrapper.java <https://reviews.apache.org/r/7043/#comment24463> How about using guava Files: File tmpExternalDir = Files.createTempDir(); http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/io/Files.html#createTempDir() hcatalog-pig-adapter/src/test/java/org/apache/hcatalog/pig/TestHCatStorerWrapper.java <https://reviews.apache.org/r/7043/#comment24464> How about using an assert on a single line: Assert.assertEquals(0, driver.run(cmd).getResponseCode()) hcatalog-pig-adapter/src/test/java/org/apache/hcatalog/pig/TestHCatStorerWrapper.java <https://reviews.apache.org/r/7043/#comment24466> Let's use HCatBaseTest.logAndRegister() which improves logging and may help someone who's debugging this in the future. src/java/org/apache/hcatalog/common/HCatConstants.java <https://reviews.apache.org/r/7043/#comment24467> Please include a javadoc comment describing this property. HCAT_DATA_CONVERT_BOOLEAN_TO_INTEGER has what I think is a good example. src/java/org/apache/hcatalog/mapreduce/FileOutputCommitterContainer.java <https://reviews.apache.org/r/7043/#comment24469> This touches something I don't fully understand about Hive external tables. One would think this test should check that the TableType is external. For example: table.getTableType() == TableType.EXTERNAL_TABLE However, as you show here the "EXTERNAL" property must also be set. We see why this is by looking at: http://svn.apache.org/viewvc/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java?view=markup 841 String tableType = mtbl.getTableType(); 842 if (tableType == null) { 843 // for backwards compatibility with old metastore persistence 844 if (mtbl.getViewOriginalText() != null) { 845 tableType = TableType.VIRTUAL_VIEW.toString(); 846 } else if ("TRUE".equals(mtbl.getParameters().get("EXTERNAL"))) { 847 tableType = TableType.EXTERNAL_TABLE.toString(); 848 } else { 849 tableType = TableType.MANAGED_TABLE.toString(); 850 } 851 } Which does check the property. So I think this is okay. - Travis Crawford On Sept. 11, 2012, 9:59 p.m., Feng Peng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7043/ > ----------------------------------------------------------- > > (Updated Sept. 11, 2012, 9:59 p.m.) > > > Review request for hcatalog, Dmitriy Ryaboy and Travis Crawford. > > > Description > ------- > > We added a property called HCAT_PIG_STORER_EXTERNAL_LOCATION to the > UDFContext of HCatStorer: when the property is set AND the table type is > EXTERNAL, HCatStorer writes the data to the external path the property > specifies. > > > This addresses bug hcatalog-500. > https://issues.apache.org/jira/browse/hcatalog-500 > > > Diffs > ----- > > hcatalog-pig-adapter/src/main/java/org/apache/hcatalog/pig/HCatStorer.java > c46db33 > > hcatalog-pig-adapter/src/test/java/org/apache/hcatalog/pig/HCatStorerWrapper.java > PRE-CREATION > > hcatalog-pig-adapter/src/test/java/org/apache/hcatalog/pig/TestHCatStorerWrapper.java > PRE-CREATION > src/java/org/apache/hcatalog/common/HCatConstants.java 626d91b > src/java/org/apache/hcatalog/mapreduce/FileOutputCommitterContainer.java > 8e692dc > src/java/org/apache/hcatalog/mapreduce/FosterStorageHandler.java 3f368d8 > > Diff: https://reviews.apache.org/r/7043/diff/ > > > Testing > ------- > > Added a corresponding unit test. Also tested on our cluster. > > > Thanks, > > Feng Peng > >
