----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56290/#review164360 -----------------------------------------------------------
accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/AccumuloDefaultIndexScanner.java (line 43) <https://reviews.apache.org/r/56290/#comment236044> A few questions: * What does "column_column" mean? Should it just be "column_name"? * "table rowid" -- does this mean the qualifier includes the base table name in addition to the rowId? * Is the visibility component just a push-down of what was included on the data-table mutation? accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/AccumuloDefaultIndexScanner.java (line 48) <https://reviews.apache.org/r/56290/#comment236045> What happens if I provide an `accumulo.indextable.name` but no `accumulo.indexed.columns`? accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/AccumuloDefaultIndexScanner.java (line 50) <https://reviews.apache.org/r/56290/#comment236046> Would suggest `accumulo.index.max.rows` for some more similarity with the other properties. We should also include description here that states that this number will affect the amount of memory necessary. accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/AccumuloDefaultIndexScanner.java (line 52) <https://reviews.apache.org/r/56290/#comment236047> Defaults to this class, yes? Would suggest including that in the docs. accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/AccumuloDefaultIndexScanner.java (line 105) <https://reviews.apache.org/r/56290/#comment236056> What's the reasoning behind setting isolation on the scanner? accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/AccumuloDefaultIndexScanner.java (line 106) <https://reviews.apache.org/r/56290/#comment236057> Oh, this is what your javadoc was saying? Why is this duplicated? accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/AccumuloDefaultIndexScanner.java (line 122) <https://reviews.apache.org/r/56290/#comment236065> I'm worried about the use of `NO_MATCH` like this. There's always the possibility of this special `Range` causing troubles due to the user-data. Is there a reason this can't just be an empty list? accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/AccumuloDefaultIndexScanner.java (line 131) <https://reviews.apache.org/r/56290/#comment236058> Please include a real error message (e.g. "Failed to scan index table"). accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/AccumuloIndexParameters.java (line 39) <https://reviews.apache.org/r/56290/#comment236060> Looks like this is un-used? accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/AccumuloIndexParameters.java (line 62) <https://reviews.apache.org/r/56290/#comment236061> Probably want to strip the leading/trailing whitespace on each element. accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/AccumuloIndexParameters.java (line 89) <https://reviews.apache.org/r/56290/#comment236062> I think this should propagate an exception. The user specified something we could not load -- this should fail-hard. accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/AccumuloIndexScanner.java (line 29) <https://reviews.apache.org/r/56290/#comment236063> Please add javadoc for each of these methods. Since they're intended for extension, the developer needs to know what each of these methods should do. accumulo-handler/src/test/org/apache/hadoop/hive/accumulo/TestAccumuloIndexParameters.java (line 32) <https://reviews.apache.org/r/56290/#comment236066> Would recommend just creating an inner-class that `implements AccumuloIndexScanner` instead of re-using the Test class itself. accumulo-handler/src/test/org/apache/hadoop/hive/accumulo/predicate/TestAccumuloRangeGenerator.java (line 476) <https://reviews.apache.org/r/56290/#comment236067> Can you please add a test here for some types other than String? A number. A boolean, maybe. This is a good start, Mike. Lots of little feedback from me :) - Josh Elser On Feb. 6, 2017, 4:51 p.m., Mike Fagan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56290/ > ----------------------------------------------------------- > > (Updated Feb. 6, 2017, 4:51 p.m.) > > > Review request for hive. > > > Repository: hive-git > > > Description > ------- > > HIVE-15795: Add Accumulo Index Table Support > > > Diffs > ----- > > > accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/AccumuloDefaultIndexScanner.java > PRE-CREATION > > accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/AccumuloIndexParameters.java > PRE-CREATION > > accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/AccumuloIndexScanner.java > PRE-CREATION > > accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/predicate/AccumuloPredicateHandler.java > a7ec7c5 > > accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/predicate/AccumuloRangeGenerator.java > 21392d1 > > accumulo-handler/src/test/org/apache/hadoop/hive/accumulo/TestAccumuloDefaultIndexScanner.java > PRE-CREATION > > accumulo-handler/src/test/org/apache/hadoop/hive/accumulo/TestAccumuloIndexParameters.java > PRE-CREATION > > accumulo-handler/src/test/org/apache/hadoop/hive/accumulo/predicate/TestAccumuloPredicateHandler.java > 88e4530 > > accumulo-handler/src/test/org/apache/hadoop/hive/accumulo/predicate/TestAccumuloRangeGenerator.java > 339da07 > > Diff: https://reviews.apache.org/r/56290/diff/ > > > Testing > ------- > > > Thanks, > > Mike Fagan > >
