----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10099/#review18330 -----------------------------------------------------------
Overall, this looks like a nice contribution. My review was fairly superficial. Many of the comments regard esthetics rather than content. A few regard basic readability. That said, it would really help to have some more high level comments that explain what connects to what. This is key for Drill since we need to make it easy for new contributors to jump in. sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/HBaseStorageEngine.java <https://reviews.apache.org/r/10099/#comment38524> This comment says nothing. It seems like there is something important to say here. The text from the JIRA might make a good start. sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/HBaseStorageEngine.java <https://reviews.apache.org/r/10099/#comment38522> This is hard to read (looks automatically generated). It should be easy to simply this automatically. It shouldn't be any different in terms of speed. sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/HBaseStorageEngine.java <https://reviews.apache.org/r/10099/#comment38523> Monster long line. Can this be broken? sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/region/HBaseRegionRecordReader.java <https://reviews.apache.org/r/10099/#comment38525> Surely there is more to say here?! Why is collocation required? Is local join really supported? sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/table/HBaseTableRecordReader.java <https://reviews.apache.org/r/10099/#comment38526> What is the difference here from the HBase*Scanner? How do they work together? sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/values/BytesDataValue.java <https://reviews.apache.org/r/10099/#comment38527> Comments like this don't add much value. Better to say something the code doesn't say. sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/values/BytesDataValue.java <https://reviews.apache.org/r/10099/#comment38528> BytesDataValue extends BaseDataValue ?! I didn't even know you could do this. sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/values/HBaseColumnValue.java <https://reviews.apache.org/r/10099/#comment38529> spelling sandbox/prototype/contrib/storage-hbase/src/test/java/org/apache/drill/hbase/HBaseStorageEngineSystemTest.java <https://reviews.apache.org/r/10099/#comment38530> There are several blank lines like this. It would be nice to clean them up. This is purely esthetic. sandbox/prototype/contrib/storage-hbase/src/test/resources/simple_donuts.json <https://reviews.apache.org/r/10099/#comment38531> Why the haphazard indentation here? - Ted Dunning On March 24, 2013, 8:45 p.m., David Alves wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10099/ > ----------------------------------------------------------- > > (Updated March 24, 2013, 8:45 p.m.) > > > Review request for drill. > > > Description > ------- > > Runs agains HBase at the table level, allows full table scans (no pushdowns). > > > Diffs > ----- > > sandbox/prototype/contrib/storage-hbase/pom.xml f42853e > > sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/HBaseResultRecordPointer.java > PRE-CREATION > > sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/HBaseStorageEngine.java > PRE-CREATION > > sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/HbaseUtils.java > PRE-CREATION > > sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/region/HBaseRegionRecordReader.java > PRE-CREATION > > sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/table/HBaseTableRecordReader.java > PRE-CREATION > > sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/table/HBaseTableScanner.java > PRE-CREATION > > sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/values/BytesDataValue.java > PRE-CREATION > > sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/values/HBaseColumnValue.java > PRE-CREATION > > sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/values/HBaseFamilyValue.java > PRE-CREATION > > sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/values/HBaseResultValue.java > PRE-CREATION > > sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/values/ImmutableHBaseMapValue.java > PRE-CREATION > > sandbox/prototype/contrib/storage-hbase/src/main/resources/drill-module.conf > PRE-CREATION > > sandbox/prototype/contrib/storage-hbase/src/test/java/org/apache/drill/hbase/HBaseStorageEngineSystemTest.java > PRE-CREATION > > sandbox/prototype/contrib/storage-hbase/src/test/java/org/apache/drill/hbase/HBaseTableRecordReaderTest.java > PRE-CREATION > sandbox/prototype/contrib/storage-hbase/src/test/resources/hbase-site.xml > PRE-CREATION > sandbox/prototype/contrib/storage-hbase/src/test/resources/log4j.properties > PRE-CREATION > > sandbox/prototype/contrib/storage-hbase/src/test/resources/simple_donuts.json > PRE-CREATION > > sandbox/prototype/contrib/storage-hbase/src/test/resources/simple_hbase_table_plan.json > PRE-CREATION > > Diff: https://reviews.apache.org/r/10099/diff/ > > > Testing > ------- > > > Thanks, > > David Alves > >
