[ 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)