hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jonathan Gray (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-3562) ValueFilter is being evaluated before performing the column match
Date Fri, 01 Apr 2011 17:05:05 GMT

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

Jonathan Gray commented on HBASE-3562:

Thanks for looking into this Evert.  This is definitely some tricky stuff.

A few comments on your patch...

- Our convention in conditionals is to put the variable first.  I find it a little tricky
to read the code when the constant is first.  For example:
if (MatchCode.INCLUDE == mc)
should be
if (mc == MatchCode.INCLUDE)
(And all the other places where you have this type of logic)

- The unit test {{TestColumnMatchAndFilterOrder}} is clever how you check correctness, but
I think it would be good to actually do a read query and verify the results for a few different
combinations of the query to prove correctness of the overall algorithm.  Other changes to
SQM down the road might change more behavior / order of operations, so this test may no longer
apply or give full coverage for correctness.  Having some tests which don't rely on the precise
server-side interactions but rather confirm the end results will be more applicable as we
move forward.

- You have some lines that are > 80 characters, especially in some of the javadoc.  Just
wrap that so all lines are <= 80 chars.

- There was a comment in SQM that described why the filter was checked first.  Can you write
some inline comments to describe how this works now?  There are a couple lines at the end
but it will be useful to have some explanation on why this has changed and what the behavior
is now.

- Is there any particular reason that you had includeLatestColumn take timestamp as a parameter?
 The timestamp is passed in the check call, and we could just hang on to that.  It just feels
a little strange to me since you should never pass a different timestamp, and the tracker
can know which was the latest column.

Overall this is really solid!  Great work Evert!

> ValueFilter is being evaluated before performing the column match
> -----------------------------------------------------------------
>                 Key: HBASE-3562
>                 URL: https://issues.apache.org/jira/browse/HBASE-3562
>             Project: HBase
>          Issue Type: Bug
>          Components: filters
>    Affects Versions: 0.90.0
>            Reporter: Evert Arckens
>         Attachments: HBASE-3562.patch
> When performing a Get operation where a both a column is specified and a ValueFilter,
the ValueFilter is evaluated before making the column match as is indicated in the javadoc
of Get.setFilter()  : " {@link Filter#filterKeyValue(KeyValue)} is called AFTER all tests
for ttl, column match, deletes and max versions have been run. "
> The is shown in the little test below, which uses a TestComparator extending a WritableByteArrayComparable.
> public void testFilter() throws Exception {
> 	byte[] cf = Bytes.toBytes("cf");
> 	byte[] row = Bytes.toBytes("row");
> 	byte[] col1 = Bytes.toBytes("col1");
> 	byte[] col2 = Bytes.toBytes("col2");
> 	Put put = new Put(row);
> 	put.add(cf, col1, new byte[]{(byte)1});
> 	put.add(cf, col2, new byte[]{(byte)2});
> 	table.put(put);
> 	Get get = new Get(row);
> 	get.addColumn(cf, col2); // We only want to retrieve col2
> 	TestComparator testComparator = new TestComparator();
> 	Filter filter = new ValueFilter(CompareOp.EQUAL, testComparator);
> 	get.setFilter(filter);
> 	Result result = table.get(get);
> }
> public class TestComparator extends WritableByteArrayComparable {
>     /**
>      * Nullary constructor, for Writable
>      */
>     public TestComparator() {
>         super();
>     }
>     @Override
>     public int compareTo(byte[] theirValue) {
>         if (theirValue[0] == (byte)1) {
>             // If the column match was done before evaluating the filter, we should never
get here.
>             throw new RuntimeException("I only expect (byte)2 in col2, not (byte)1 from
>         }
>         if (theirValue[0] == (byte)2) {
>             return 0;
>         }
>         else return 1;
>     }
> }
> When only one column should be retrieved, this can be worked around by using a SingleColumnValueFilter
instead of the ValueFilter.

This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira

View raw message