Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201611041440.uA4EeDx4023453@ip-10-146-233-104.ec2.internal>
Date: Fri, 4 Nov 2016 14:40:13 +0000
From: "Alex Behm (Code Review)" <gerrit@cloudera.org>
To: Tim Armstrong <tarmstrong@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Bharath Vissapragada <bharathv@cloudera.com>
Reply-To: alex.behm@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4266=3A_Java_udf_returning_string_can_give_incorrect_results=0A?=
In-Reply-To: <gerrit.1478221322000.I705d271814cb1143f67d8a12f4fd87bab7a8e161@gerrit.cloudera.org>
References: <gerrit.1478221322000.I705d271814cb1143f67d8a12f4fd87bab7a8e161@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Fri, 04 Nov 2016 14:41:22 -0000

Alex Behm has posted comments on this change.

Change subject: IMPALA-4266: Java udf returning string can give incorrect results
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4941/1/be/src/exprs/hive-udf-call.cc
File be/src/exprs/hive-udf-call.cc:

Line 334:   FunctionContext* fn_ctx = ctx->fn_context(fn_context_index_);
Just wondering whether you've considered the following alternative: Take the native allocation made from the FE UDF Eval, stick the ptr in our local allocations and then let those be cleaned up as usual in FreeLocalAllocations(). We'd have to allocate a new result buffer for every result in UDFExecutor.java like we do for regular exprs, but we can pool the allocation. That would save us copying the string.

Not sure it's worth it, but wanted to mention it.

In any case it would be useful to comment that the underlying StringVal buffer is allocated from the FE using Unsafe, and that the UDF evaluator reuses that buffer for subsequent invokations.


-- 
To view, visit http://gerrit.cloudera.org:8080/4941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I705d271814cb1143f67d8a12f4fd87bab7a8e161
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-HasComments: Yes