[ https://issues.apache.org/jira/browse/HBASE-4176?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13085209#comment-13085209 ]
jirapos...@reviews.apache.org commented on HBASE-4176: ------------------------------------------------------ ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1326/#review1457 ----------------------------------------------------------- just did a first pass review. a few overall notes: - throughout, style nit: no space between function name and opening paren: void foo(Bar baz), not void foo (Bar baz) - try to avoid spurious whitespace changes /src/main/java/org/apache/hadoop/hbase/filter/ColumnCountGetFilter.java <https://reviews.apache.org/r/1326/#comment3356> style: no space before '(' /src/main/java/org/apache/hadoop/hbase/filter/Filter.java <https://reviews.apache.org/r/1326/#comment3364> it seems strange that this returns Filter, but isn't a static method. It seems to me it should either be a static method, accessed via reflection, or it should be non-static, and mutate the existing filter object (like readFields does) /src/main/java/org/apache/hadoop/hbase/filter/MultipleColumnPrefixFilter.java <https://reviews.apache.org/r/1326/#comment3363> spurious whitespace changes /src/main/java/org/apache/hadoop/hbase/filter/ParseFilter.java <https://reviews.apache.org/r/1326/#comment3357> this function's name is kind of inconsistent - it doesn't return a String. /src/main/resources/org/apache/hadoop/hbase/thrift/Hbase.thrift <https://reviews.apache.org/r/1326/#comment3358> lots of spurious whitespace changes in this file, please remove /src/main/resources/org/apache/hadoop/hbase/thrift/Hbase.thrift <https://reviews.apache.org/r/1326/#comment3359> rather than a combinatorial expansion of the various scanner open options, what if we just had something like: struct ScannerSpec { 1:optional Text startRow 2:optional Text stopRow 3:optional Text filterString 4:optional i64 maxTimestamp ... } then we can add more of these composable options without adding the power-set of "scannerOpen*" functions /src/main/ruby/shell/commands/scan.rb <https://reviews.apache.org/r/1326/#comment3360> if we can commit this document to be built into the docs dir as part of the build, that would be preferable. Some people use HBase in an environment where they do not have internet access, and others have no idea where the JIRA might be. /src/test/java/org/apache/hadoop/hbase/filter/TestParseFilter.java <https://reviews.apache.org/r/1326/#comment3361> this is no good - if you get the IAE, the test will still pass, which is not what you want at all. Change the test cases to throw Exception instead, and let it bubble through to fail the test case. all of these test cases are also the same, why not refactor into: private static T doTestFilter(String filterString, Class<T extends FilterBase> clazz) { ... return clazz.cast(filter); } which does the construction, assertion of the correct class being created, etc? then any filter-specific assertions can be done on the return value - Todd On 2011-08-11 18:09:36, Anirudh Todi wrote: bq. bq. ----------------------------------------------------------- bq. This is an automatically generated e-mail. To reply, visit: bq. https://reviews.apache.org/r/1326/ bq. ----------------------------------------------------------- bq. bq. (Updated 2011-08-11 18:09:36) bq. bq. bq. Review request for hbase, Todd Lipcon, Ted Yu, Michael Stack, and Jonathan Gray. bq. bq. bq. Summary bq. ------- bq. bq. https://issues.apache.org/jira/browse/HBASE-4176: Exposing HBase Filters to the Thrift API bq. bq. Currently, to use any of the filters, one has to explicitly add a scanner for the filter in the Thrift API making it messy and long. bq. With this patch, I am trying to add support for all the filters in a clean way. bq. The user specifies a filter via a string. The string is parsed on the server to construct the filter. More information can be found in the attached document named Filter Language bq. bq. This patch is trying to extend and further the progress made by the patches in HBASE-1744 bq. bq. There is document attached to the HBASE-4176 JIRA that describes this patch in further detail bq. bq. bq. This addresses bug HBASE-4176. bq. https://issues.apache.org/jira/browse/HBASE-4176 bq. bq. bq. Diffs bq. ----- bq. bq. /src/main/java/org/apache/hadoop/hbase/filter/ColumnCountGetFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/ColumnPaginationFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/ColumnPrefixFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/ColumnRangeFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/CompareFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/DependentColumnFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/FamilyFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/Filter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/FilterBase.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/FilterList.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/FirstKeyOnlyFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/InclusiveStopFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/KeyOnlyFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/MultipleColumnPrefixFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/PageFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/ParseConstants.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/ParseFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/PrefixFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/QualifierFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/RowFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/SingleColumnValueExcludeFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/SingleColumnValueFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/TimestampsFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/filter/ValueFilter.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/thrift/ThriftServer.java 1156439 bq. /src/main/java/org/apache/hadoop/hbase/thrift/generated/Hbase.java 1156439 bq. /src/main/resources/org/apache/hadoop/hbase/thrift/Hbase.thrift 1156439 bq. /src/main/ruby/hbase/table.rb 1156439 bq. /src/main/ruby/shell/commands/scan.rb 1156439 bq. /src/test/java/org/apache/hadoop/hbase/filter/TestParseFilter.java 1156439 bq. bq. Diff: https://reviews.apache.org/r/1326/diff bq. bq. bq. Testing bq. ------- bq. bq. patch includes one test: TestParseFilter.java bq. bq. bq. Thanks, bq. bq. Anirudh bq. bq. > Exposing HBase Filters to the Thrift API > ---------------------------------------- > > Key: HBASE-4176 > URL: https://issues.apache.org/jira/browse/HBASE-4176 > Project: HBase > Issue Type: Improvement > Components: thrift > Reporter: Anirudh Todi > Assignee: Anirudh Todi > Priority: Minor > Attachments: Filter Language(2).docx, Filter Language.docx, > HBASE-4176.patch > > > Currently, to use any of the filters, one has to explicitly add a scanner for > the filter in the Thrift API making it messy and long. With this patch, I am > trying to add support for all the filters in a clean way. The user specifies > a filter via a string. The string is parsed on the server to construct the > filter. More information can be found in the attached document named Filter > Language > This patch is trying to extend and further the progress made by the patches > in the HBASE-1744 JIRA (https://issues.apache.org/jira/browse/HBASE-1744) -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira