Mailing-List: contact dev-help@phoenix.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@phoenix.apache.org
Date: Wed, 31 May 2017 22:53:04 +0000 (UTC)
From: "James Taylor (JIRA)" <jira@apache.org>
To: dev@phoenix.apache.org
Message-ID: <JIRA.13062584.1491605919000.336718.1496271184099@Atlassian.JIRA>
In-Reply-To: <JIRA.13062584.1491605919000@Atlassian.JIRA>
References: <JIRA.13062584.1491605919000@Atlassian.JIRA> <JIRA.13062584.1491605919798@jira-lw-us.apache.org>
Subject: [jira] [Commented] (PHOENIX-3773) Implement FIRST_VALUES aggregate
 function
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
archived-at: Wed, 31 May 2017 22:53:09 -0000


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

James Taylor commented on PHOENIX-3773:
---------------------------------------

Thanks for the updated patch, [~singamteja]. It's looking very good. Here's some feedback:

Instead of deserializing the byte arrays to objects (a relatively expensive operation) in the evaluate method of FirstLastValueBaseClientAggregator, store the bytes pointer in a List<ImmutableBytesWritable>.
{code}
+                    if (getMultipleValues) {
+                        multipleValuesResult.add(PDataType.fromTypeId(dataType.getSqlType() - PDataType.ARRAY_TYPE_BASE).toObject(it.next()));
+                        if (++counter == offset) {
+                            break;
+                        }
+                    } else {
{code}
Then, once you have all the values, combine them together using the PArrayDataType.appendItemToArray() method outside the loop.
{code}
-            //not enought values to return Nth
+            if (getMultipleValues && multipleValuesResult.size() > 0) {
+                ptr.set(dataType.toBytes(PArrayDataType.instantiatePhoenixArray(PDataType.fromTypeId(dataType.getSqlType() - PDataType.ARRAY_TYPE_BASE), multipleValuesResult.toArray())));
+                return true;
+            }
+
+            //not enough values to return Nth or top N'values or trying to retreive (N+1)th value
{code}
I think the handling of the case in which there are no values may not be handled correctly. You need to move this check to the top of the aggregate method:
{code}
        if (topValue == null) {
            return false;
        }
{code}
And this would be the only case that would return false. Otherwise, if there aren't enough values, do this:
{code}
    ptr.set(ByteUtil.EMPTY_BYTE_ARRAY);
    return true;
{code}
Probably a good idea to have a test that asks for the top 3 values when there are only 2 values to make sure that case works too (if you don't have that already).

Also, one minor nit. How about instead of calling the new member variable getMultipleValues you call it isArrayReturnType?

> Implement FIRST_VALUES aggregate function
> -----------------------------------------
>
>                 Key: PHOENIX-3773
>                 URL: https://issues.apache.org/jira/browse/PHOENIX-3773
>             Project: Phoenix
>          Issue Type: New Feature
>            Reporter: James Taylor
>            Assignee: Loknath Priyatham Teja Singamsetty 
>              Labels: SFDC
>             Fix For: 4.11.0
>
>         Attachments: PHOENIX-3773_4.x-HBase-0.98.patch, PHOENIX-3773_master.patch, PHOENIX-3773.patch, PHOENIX-3773.v2.patch, PHOENIX-3773.v3.patch
>
>
> Similar to FIRST_VALUE, but would allow the user to specify how many values to keep. This could use a MinMaxPriorityQueue under the covers and be much more efficient than using multiple NTH_VALUE calls to do the same like this:
> {code}
> SELECT entity_id,
>        NTH_VALUE(user_id,1) WITHIN GROUP (ORDER BY last_read_date DESC) as nth1_user_id,
>        NTH_VALUE(user_id,2) WITHIN GROUP (ORDER BY last_read_date DESC) as nth2_user_id,
>        NTH_VALUE(user_id,3) WITHIN GROUP (ORDER BY last_read_date DESC) as nth3_user_id,
>        count(*)
> FROM  MY_TABLE 
> WHERE tenant_id='00Dx0000000XXXX'
> AND entity_id in ('0D5x000000ABCD','0D5x000000ABCE')
> GROUP BY entity_id;
> {code}


--
This message was sent by Atlassian JIRA
(v6.3.15#6346)