HBase
  1. HBase
  2. HBASE-5821

Incorrect handling of null value in Coprocessor aggregation function min()

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.92.1
    • Fix Version/s: 0.92.2
    • Component/s: Coprocessors
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Both in AggregateImplementation and AggregationClient, the evaluation of the current minimum value is like:
      min = (min == null || ci.compare(result, min) < 0) ? result : min;

      The LongColumnInterpreter takes null value is treated as the least value, while the above expression takes min as the greater value when it is null. Thus, the real minimum value gets discarded if a null value comes later.
      max() could also be wrong if a different ColumnInterpreter other than LongColumnInterpreter treats null value differently (as the greatest).

        Activity

        Hide
        Maryann Xue added a comment -

        Add null value check before calling ColumnInterpreter.compare()

        Show
        Maryann Xue added a comment - Add null value check before calling ColumnInterpreter.compare()
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12523202/HBASE-5821.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 6 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1566//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1566//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1566//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12523202/HBASE-5821.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 6 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1566//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1566//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1566//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        Patch makes sense.

        Show
        Ted Yu added a comment - Patch makes sense.
        Hide
        Himanshu Vashishtha added a comment -

        Yes, its a bug. Thanks for pointing this out.
        I think the change should be done at the LongColumnInterpreter level?

        Show
        Himanshu Vashishtha added a comment - Yes, its a bug. Thanks for pointing this out. I think the change should be done at the LongColumnInterpreter level?
        Hide
        Maryann Xue added a comment -

        LongColumnInterpreter is simply an implementation of ColumnInterpreter, and there could be any other user-defined ColumnInterpreter, so we should not have any assumption here. It is better to guarantee the caller itself handles the logic properly.

        Show
        Maryann Xue added a comment - LongColumnInterpreter is simply an implementation of ColumnInterpreter, and there could be any other user-defined ColumnInterpreter, so we should not have any assumption here. It is better to guarantee the caller itself handles the logic properly.
        Hide
        Himanshu Vashishtha added a comment -

        Yeah, LCM is just an implementation and I thought to better keep the semantics of CI right and tight such that we just call compare and it gives what we want (as per CI.compareTo() javadoc).
        Otherwise, I am good.

        Show
        Himanshu Vashishtha added a comment - Yeah, LCM is just an implementation and I thought to better keep the semantics of CI right and tight such that we just call compare and it gives what we want (as per CI.compareTo() javadoc). Otherwise, I am good.
        Hide
        Ted Yu added a comment -

        @Himanshu:
        The LCM above refers to LongColumnInterpreter, right ?
        Are you fine with Maryann's patch ?

        Show
        Ted Yu added a comment - @Himanshu: The LCM above refers to LongColumnInterpreter, right ? Are you fine with Maryann's patch ?
        Hide
        Himanshu Vashishtha added a comment -

        Yes, I meant LongColumnInterpreter. There is a minor nit.

        As per the existing code, max is correct (it will not override the value with an incoming null result); but min is buggy. Maryann said that too.

        I think we should have a uniform check at both the places (Client and Protocol) ie. in client we are checking :

                max = ci.compare(max, result) < 0 ? result : max;
        

        and in Protocol we are checking:

                  max = (max == null || ci.compare(temp, max) > 0) ? temp : max;
        

        The ordering of arguments in the compare method is not same.

        That was also why I said that its good to have these sort of check at single place (and Column interpreter implementations are good candidate for that as they can define their own way to handle them. They just have to follow the Interface contract.).

        Anyway, if we go with the existing one, I think we should use the same order just to avoid any confusion.
        I know its a minor nit, but it would be good to have a more consistent code?

        Thanks for looking into this.

        Show
        Himanshu Vashishtha added a comment - Yes, I meant LongColumnInterpreter. There is a minor nit. As per the existing code, max is correct (it will not override the value with an incoming null result); but min is buggy. Maryann said that too. I think we should have a uniform check at both the places (Client and Protocol) ie. in client we are checking : max = ci.compare(max, result) < 0 ? result : max; and in Protocol we are checking: max = (max == null || ci.compare(temp, max) > 0) ? temp : max; The ordering of arguments in the compare method is not same. That was also why I said that its good to have these sort of check at single place (and Column interpreter implementations are good candidate for that as they can define their own way to handle them. They just have to follow the Interface contract.). Anyway, if we go with the existing one, I think we should use the same order just to avoid any confusion. I know its a minor nit, but it would be good to have a more consistent code? Thanks for looking into this.
        Hide
        Maryann Xue added a comment -

        Himanshu, it seems impossible to solve this problem through ColumnInterpreter. the semantics of min() and max() here is to discard null value whenever a non-null value appears; while the compare() is put the null value at a definite end – either the lowest or the highest. Currently, null is treated as the smallest value, so this works for max(), but for min(), null would be the minimum value instead of being discarded.

        Show
        Maryann Xue added a comment - Himanshu, it seems impossible to solve this problem through ColumnInterpreter. the semantics of min() and max() here is to discard null value whenever a non-null value appears; while the compare() is put the null value at a definite end – either the lowest or the highest. Currently, null is treated as the smallest value, so this works for max(), but for min(), null would be the minimum value instead of being discarded.
        Hide
        Himanshu Vashishtha added a comment -

        I see your point about different use cases.
        With the current patch we are invoking the comparator method only when both arguments are non-null.
        Anyway, its working correctly now, I am good
        Thanks for correcting it Maryann.

        Show
        Himanshu Vashishtha added a comment - I see your point about different use cases. With the current patch we are invoking the comparator method only when both arguments are non-null. Anyway, its working correctly now, I am good Thanks for correcting it Maryann.
        Hide
        Ted Yu added a comment -

        Integrated to 0.92, 0.94 and trunk.

        Thanks for the patch, Maryann.

        Thanks for the review Himanshu.

        Show
        Ted Yu added a comment - Integrated to 0.92, 0.94 and trunk. Thanks for the patch, Maryann. Thanks for the review Himanshu.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2786 (See https://builds.apache.org/job/HBase-TRUNK/2786/)
        HBASE-5821 Incorrect handling of null value in Coprocessor aggregation function min() (Maryann Xue) (Revision 1328030)

        Result = FAILURE
        tedyu :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/coprocessor/AggregationClient.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2786 (See https://builds.apache.org/job/HBase-TRUNK/2786/ ) HBASE-5821 Incorrect handling of null value in Coprocessor aggregation function min() (Maryann Xue) (Revision 1328030) Result = FAILURE tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/coprocessor/AggregationClient.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12523202/HBASE-5821.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1577//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12523202/HBASE-5821.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1577//console This message is automatically generated.
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94 #133 (See https://builds.apache.org/job/HBase-0.94/133/)
        HBASE-5821 Incorrect handling of null value in Coprocessor aggregation function min() (Maryann Xue) (Revision 1328027)

        Result = FAILURE
        tedyu :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/coprocessor/AggregationClient.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java
        Show
        Hudson added a comment - Integrated in HBase-0.94 #133 (See https://builds.apache.org/job/HBase-0.94/133/ ) HBASE-5821 Incorrect handling of null value in Coprocessor aggregation function min() (Maryann Xue) (Revision 1328027) Result = FAILURE tedyu : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/coprocessor/AggregationClient.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.92 #378 (See https://builds.apache.org/job/HBase-0.92/378/)
        HBASE-5821 Incorrect handling of null value in Coprocessor aggregation function min() (Maryann Xue) (Revision 1328025)

        Result = FAILURE
        tedyu :
        Files :

        • /hbase/branches/0.92/CHANGES.txt
        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/coprocessor/AggregationClient.java
        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java
        Show
        Hudson added a comment - Integrated in HBase-0.92 #378 (See https://builds.apache.org/job/HBase-0.92/378/ ) HBASE-5821 Incorrect handling of null value in Coprocessor aggregation function min() (Maryann Xue) (Revision 1328025) Result = FAILURE tedyu : Files : /hbase/branches/0.92/CHANGES.txt /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/coprocessor/AggregationClient.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94-security #17 (See https://builds.apache.org/job/HBase-0.94-security/17/)
        HBASE-5821 Incorrect handling of null value in Coprocessor aggregation function min() (Maryann Xue) (Revision 1328027)

        Result = FAILURE
        tedyu :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/coprocessor/AggregationClient.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java
        Show
        Hudson added a comment - Integrated in HBase-0.94-security #17 (See https://builds.apache.org/job/HBase-0.94-security/17/ ) HBASE-5821 Incorrect handling of null value in Coprocessor aggregation function min() (Maryann Xue) (Revision 1328027) Result = FAILURE tedyu : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/coprocessor/AggregationClient.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #176 (See https://builds.apache.org/job/HBase-TRUNK-security/176/)
        HBASE-5821 Incorrect handling of null value in Coprocessor aggregation function min() (Maryann Xue) (Revision 1328030)

        Result = FAILURE
        tedyu :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/coprocessor/AggregationClient.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #176 (See https://builds.apache.org/job/HBase-TRUNK-security/176/ ) HBASE-5821 Incorrect handling of null value in Coprocessor aggregation function min() (Maryann Xue) (Revision 1328030) Result = FAILURE tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/coprocessor/AggregationClient.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.92-security #105 (See https://builds.apache.org/job/HBase-0.92-security/105/)
        HBASE-5821 Incorrect handling of null value in Coprocessor aggregation function min() (Maryann Xue) (Revision 1328025)

        Result = FAILURE
        tedyu :
        Files :

        • /hbase/branches/0.92/CHANGES.txt
        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/coprocessor/AggregationClient.java
        • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java
        Show
        Hudson added a comment - Integrated in HBase-0.92-security #105 (See https://builds.apache.org/job/HBase-0.92-security/105/ ) HBASE-5821 Incorrect handling of null value in Coprocessor aggregation function min() (Maryann Xue) (Revision 1328025) Result = FAILURE tedyu : Files : /hbase/branches/0.92/CHANGES.txt /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/coprocessor/AggregationClient.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java
        Hide
        asha koshti added a comment -

        Even i am facing the issue in using coprocessor for doing sum.

        AggregationClient aClient = new AggregationClient(conf);
        final ColumnInterpreter<Long, Long> ci = new LongColumnInterpreter();
        Scan scan = new Scan();
        scan.addColumn(Bytes.toBytes("info"),Bytes.toBytes("hits"));
        long sum = aClient.sum(Bytes.toBytes("demo"), ci, scan);

        Show
        asha koshti added a comment - Even i am facing the issue in using coprocessor for doing sum. AggregationClient aClient = new AggregationClient(conf); final ColumnInterpreter<Long, Long> ci = new LongColumnInterpreter(); Scan scan = new Scan(); scan.addColumn(Bytes.toBytes("info"),Bytes.toBytes("hits")); long sum = aClient.sum(Bytes.toBytes("demo"), ci, scan);

          People

          • Assignee:
            Maryann Xue
            Reporter:
            Maryann Xue
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development