[ 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: {code} if (MatchCode.INCLUDE == mc) {code} should be {code} if (mc == MatchCode.INCLUDE) {code} (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 col1"); > } > 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