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

Himanshu Vashishtha commented on HBASE-5821:
--------------------------------------------

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 :
{code}
        max = ci.compare(max, result) < 0 ? result : max;
{code}
and in Protocol we are checking:
{code}
          max = (max == null || ci.compare(temp, max) > 0) ? temp : max;
{code}

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.
                
> Incorrect handling of null value in Coprocessor aggregation function min()
> --------------------------------------------------------------------------
>
>                 Key: HBASE-5821
>                 URL: https://issues.apache.org/jira/browse/HBASE-5821
>             Project: HBase
>          Issue Type: Bug
>          Components: coprocessors
>    Affects Versions: 0.92.1
>            Reporter: Maryann Xue
>            Assignee: Maryann Xue
>             Fix For: 0.92.2, 0.96.0, 0.94.1
>
>         Attachments: HBASE-5821.patch
>
>
> 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).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: 
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Reply via email to