----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10099/#review18462 -----------------------------------------------------------
This is a great patch. Thanks for all the work! Generally, things look great. The whole addField/removeField thing needs to be solved. Either here or by removing those methods and changing the operators to rely on value level copies. Can you open a jira on that and have this depend on those changes? sandbox/prototype/contrib/storage-hbase/pom.xml <https://reviews.apache.org/r/10099/#comment38689> Generally ordering of g:a:v is easier to read than a:g:v sandbox/prototype/contrib/storage-hbase/pom.xml <https://reviews.apache.org/r/10099/#comment38690> Same as above sandbox/prototype/contrib/storage-hbase/pom.xml <https://reviews.apache.org/r/10099/#comment38691> In general, I think we'll be depending on hadoop-core and hbase in multiple locations. It might be better to add it to the dependency management section of the root pom and then just reference it here. That way we don't have to keep repeating all the exclusions in different poms. sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/HBaseResultRecordPointer.java <https://reviews.apache.org/r/10099/#comment38692> We need to either support these or need to update all the reference operators to always use value copying as opposed to record modification sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/HBaseResultRecordPointer.java <https://reviews.apache.org/r/10099/#comment38693> Same as previous sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/HBaseStorageEngine.java <https://reviews.apache.org/r/10099/#comment38694> Nice! sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/HBaseStorageEngine.java <https://reviews.apache.org/r/10099/#comment38695> It seems like the frequent use case is people using values. Having them be required to type .value every time seems a bit challenging. Not sure what the best approach would be for timestamps though. Maybe something simple like table.cf.qualifier is the value and table.cf.@qualifier is the timestamp? Also, do you have any thought on how multiple versions might be managed? sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/HbaseUtils.java <https://reviews.apache.org/r/10099/#comment38696> Cleaner to use Charsets.* from Guava so you don't have to manage UnsupportedEncodingException sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/HbaseUtils.java <https://reviews.apache.org/r/10099/#comment38697> Same as above. sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/HbaseUtils.java <https://reviews.apache.org/r/10099/#comment38698> I wonder if this is common enough that we should add a DataValue.getAsBytes() as well as a DataValue.getAsBytes(ByteBuffer b) rather than have to manage these for each storage engine that likes to eat bytes. sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/values/BytesDataValue.java <https://reviews.apache.org/r/10099/#comment38699> How is the different from org.apache.drill.exec.ref.values.ScalarValues$BytesScalar? sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/values/ImmutableHBaseMapValue.java <https://reviews.apache.org/r/10099/#comment38701> Why overridden? Just to apply final? sandbox/prototype/contrib/storage-hbase/src/main/java/org/apache/drill/hbase/values/ImmutableHBaseMapValue.java <https://reviews.apache.org/r/10099/#comment38702> Same comment as elsewhere. We need to either become fully read-only for all values after created and update the rse operators to read only or we need to implement these. I'm more for moving to read only. sandbox/prototype/contrib/storage-hbase/src/test/java/org/apache/drill/hbase/HBaseStorageEngineSystemTest.java <https://reviews.apache.org/r/10099/#comment38703> A nice test to add here would be to use Jackson's intermediate JSON format and actually do a full tree iteration based comparison of the two objects. You would have to add some special logical for timestamps/leaf values. sandbox/prototype/contrib/storage-hbase/src/test/resources/log4j.properties <https://reviews.apache.org/r/10099/#comment38704> We are trying to use logback everywhere. Can you switch to that? - Jacques Nadeau On March 25, 2013, 12:19 a.m., David Alves wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10099/ > ----------------------------------------------------------- > > (Updated March 25, 2013, 12:19 a.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/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 > >
