----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8148/#review13875 -----------------------------------------------------------
core/src/main/java/org/apache/hcatalog/mapreduce/HCatInputFormat.java <https://reviews.apache.org/r/8148/#comment29659> I actually did that first, putting preconditions everywhere. While updating usages/tests I ended up allowing nulls for filters, and null for dbName. Consider the following from: https://github.com/apache/hcatalog/blob/trunk/core/src/main/java/org/apache/hcatalog/data/transfer/impl/HCatInputFormatReader.java#L66 CURRENT: InputJobInfo jobInfo = InputJobInfo.create(re.getDbName(), re.getTableName(), re.getFilterString()); HCatInputFormat.setInput(job, jobInfo); HCatInputFormat hcif = new HCatInputFormat(); WITHOUT ALLOWING NULL FILTERS: HCatInputFormat hcif = HCatInputFormat.setInput(job, re.getDbName(), re.getTableName()); if (re.getFilterString() != null) { hcif.setFilter(re.getFilterString()); } I found myself doing this in numerous places, and it really feels kludgy. So I decided to allow null filters. Also while debugging a failing test, I found a test checking explicitly that null dbName works. I puzzled over that one, because dbName is optional, so should it even be an argument to setInput or have a setDb() method? I chose to keep dbName as an argument to setInput as it feels more natural. With that additional info, are you okay with allowing null filters? I think it makes things easier for users this way. If we want to keep things consistent I could allow null properties too. This just never came up updating the tests. - Travis Crawford On Nov. 29, 2012, 1:46 a.m., Travis Crawford wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8148/ > ----------------------------------------------------------- > > (Updated Nov. 29, 2012, 1:46 a.m.) > > > Review request for hcatalog. > > > Description > ------- > > Currently we expose the following interface to use HCatInputFormat: > > """ > HCatInputFormat.setInput(job, InputJobInfo.create(dbName, inputTableName, > null)); > """ > > Notice how InputJobInfo is exposed directly to end-users. InputJobInfo should > be treated as an implementation detail, and the current API should be > deprecated and later removed. > > This change is my first pass at deprecating the above. Feedback would be > appreciated. In particular, I'm interested in what method signatures we > should provide people. At present we need the following: > > Configuration - required > database name - required > table name - required > filter - optional > properties - optional > > To keep things simple I depercated everything except a single constructor > that takes those above arguments. > > Questions: > * its cumbersome for users to say job.getConfiguration instead of passing in > a job - should we provide that convenience? > * filter is optional - should we provide a method that automatically nulls > out? > * properties is optional and rarely used (I believe only in the hbase storage > handler) - should we provide a method that automatically sets to null? > > > This addresses bug HCATALOG-527. > https://issues.apache.org/jira/browse/HCATALOG-527 > > > Diffs > ----- > > > core/src/main/java/org/apache/hcatalog/data/transfer/impl/HCatInputFormatReader.java > cf9bba2 > core/src/main/java/org/apache/hcatalog/mapreduce/HCatInputFormat.java > 7a7446f > core/src/main/java/org/apache/hcatalog/mapreduce/InitializeInput.java > e91ddc7 > core/src/main/java/org/apache/hcatalog/mapreduce/InputJobInfo.java 8779c5e > core/src/test/java/org/apache/hcatalog/data/TestReaderWriter.java 18d368f > core/src/test/java/org/apache/hcatalog/mapreduce/HCatMapReduceTest.java > 74c4888 > core/src/test/java/org/apache/hcatalog/mapreduce/TestHCatInputFormat.java > a6b381e > hcatalog-pig-adapter/src/main/java/org/apache/hcatalog/pig/HCatLoader.java > 5a4d400 > > storage-handlers/hbase/src/test/org/apache/hcatalog/hbase/TestHBaseBulkOutputFormat.java > 3875185 > > storage-handlers/hbase/src/test/org/apache/hcatalog/hbase/TestHBaseDirectOutputFormat.java > 81a3099 > > storage-handlers/hbase/src/test/org/apache/hcatalog/hbase/TestHBaseInputFormat.java > 78724a1 > > storage-handlers/hbase/src/test/org/apache/hcatalog/hbase/TestSnapshots.java > e07bf46 > > Diff: https://reviews.apache.org/r/8148/diff/ > > > Testing > ------- > > I have not run tests yet as I'm soliciting feedback on the interface change > at this time. Will run tests and our internal integration tests before > posting a final version. > > > Thanks, > > Travis Crawford > >
