[ 
https://issues.apache.org/jira/browse/HBASE-15333?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15257618#comment-15257618
 ] 

Jonathan Hsieh commented on HBASE-15333:
----------------------------------------

Here's my review on v5.

DefaultSource.scala
These replacements are fairly complicated -- more complicated that what they 
replace.  Add a comment to convert this into a verification exercise as opposed 
to a reverse engineering exercise?

DefaultSource.scala:618: typo- "righRange"

DefaultSource.scala:617-627: Make a "boolean hasOverlap(left,right)" method to 
make this easier to read?

DynamicLogicExpression.scala:28
- why are these encoding types in FilterOps?  would i tmake sense to put them 
in a separate class?
- why are we encoding these in naively using Bytes.xxxx?  this is likely a 
place we will want to have flexibility (e.g. we may want to encode data 
differently).  These encodings are not going to be any good for semantic 
sort-ordered in row keys.  This is specifically where the  data encoding 
mechanisms in hbase's OrderedBytes [1] that Nick D mentioned on the first 
comment.  Why not use that instead of coming up with yet another encoding 
scheme?  

DynamicLogicExpression.scala:200/297 Why is "Pass" changed to "dummy Pass -1"?

BoundRange.scala:26
can we use scaladoc formatting for comments where appropriate?
for bound range, are values inclusive or exclusive ranges? 

DynamicLogicExrpressionSuite.scala
- FilterOps.encode just seems strange.   Why isn't it something like 
"JavaBytesEncoder.encode" (and allow for other encodings?)

PartitionFilterSuite.scala
- I need to run the code in order to verify it.  Could we get the expected 
'show' output string and then do a strcmp instead of just a count?

[1] See 
https://github.com/apache/hbase/blob/master/hbase-common/src/main/java/org/apache/hadoop/hbase/util/OrderedBytes.java

> Enhance the filter to handle short, integer, long, float and double
> -------------------------------------------------------------------
>
>                 Key: HBASE-15333
>                 URL: https://issues.apache.org/jira/browse/HBASE-15333
>             Project: HBase
>          Issue Type: Sub-task
>            Reporter: Zhan Zhang
>            Assignee: Zhan Zhang
>         Attachments: HBASE-15333-1.patch, HBASE-15333-2.patch, 
> HBASE-15333-3.patch, HBASE-15333-4.patch, HBASE-15333-5.patch
>
>
> Currently, the range filter is based on the order of bytes. But for java 
> primitive type, such as short, int, long, double, float, etc, their order is 
> not consistent with their byte order, extra manipulation has to be in place 
> to take care of them  correctly.
> For example, for the integer range (-100, 100), the filter <= 1, the current 
> filter will return 0 and 1, and the right return value should be (-100, 1]



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to