Repository: incubator-impala Updated Branches: refs/heads/master 707f71b6e -> b82eed5ee
IMPALA-4518: CopyStringVal() doesn't copy null string Previously, CopyStringVal() mistakenly copies a null StringVal as an empty string (i.e. a non-null string with zero length). This change fixes the problem by distinguishing between these two cases in CopyStringVal() and handles them properly. Also added a test case for it. This problem only started showing up recently due to commit 51268c053ffe41dc1aa9f1b250878113d4225258 which calls CopyStringVal() in OffsetFnInit(). All other pre-existing callers of CopyStringVal() before that commit checks if 'src' is null before calling it so the problem never showed up. In that sense, this is a latent bug exposed by the aforementioned commit. Change-Id: I3a5b9349dd08556eba5cfedc8c0063cc59f5be03 Reviewed-on: http://gerrit.cloudera.org:8080/5198 Reviewed-by: Michael Ho <[email protected]> Tested-by: Internal Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/b82eed5e Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/b82eed5e Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/b82eed5e Branch: refs/heads/master Commit: b82eed5ee02eccc21cc69d4af107c0acf31e08fa Parents: 707f71b Author: Michael Ho <[email protected]> Authored: Tue Nov 22 13:48:52 2016 -0800 Committer: Internal Jenkins <[email protected]> Committed: Thu Nov 24 00:23:58 2016 +0000 ---------------------------------------------------------------------- be/src/exprs/aggregate-functions-ir.cc | 16 ++++++++++------ .../queries/QueryTest/analytic-fns.test | 10 ++++++++++ 2 files changed, 20 insertions(+), 6 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/b82eed5e/be/src/exprs/aggregate-functions-ir.cc ---------------------------------------------------------------------- diff --git a/be/src/exprs/aggregate-functions-ir.cc b/be/src/exprs/aggregate-functions-ir.cc index f6a91f4..090a905 100644 --- a/be/src/exprs/aggregate-functions-ir.cc +++ b/be/src/exprs/aggregate-functions-ir.cc @@ -147,14 +147,18 @@ static void AllocBuffer(FunctionContext* ctx, StringVal* dst, size_t buf_len) { // 'buf_len' bytes and copies the content of StringVal 'src' into it. // If allocation fails, 'dst' will be set to a null string. static void CopyStringVal(FunctionContext* ctx, const StringVal& src, StringVal* dst) { - uint8_t* copy = ctx->Allocate(src.len); - if (UNLIKELY(copy == NULL && src.len != 0)) { - DCHECK(!ctx->impl()->state()->GetQueryStatus().ok()); + if (src.is_null) { *dst = StringVal::null(); } else { - *dst = StringVal(copy, src.len); - // Avoid memcpy() to NULL ptr as it's undefined. - if (LIKELY(dst->ptr != NULL)) memcpy(dst->ptr, src.ptr, src.len); + uint8_t* copy = ctx->Allocate(src.len); + if (UNLIKELY(copy == NULL)) { + // Zero-length allocation always returns a hard-coded pointer. + DCHECK(src.len != 0 && !ctx->impl()->state()->GetQueryStatus().ok()); + *dst = StringVal::null(); + } else { + *dst = StringVal(copy, src.len); + memcpy(dst->ptr, src.ptr, src.len); + } } } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/b82eed5e/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test ---------------------------------------------------------------------- diff --git a/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test b/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test index 62c9a67..88339e6 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test +++ b/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test @@ -1907,3 +1907,13 @@ BIGINT ---- RESULTS 7299 ==== +---- QUERY +# Regression test for IMPALA-4518: Distinguishes between empty string and null string. +select f,lead(b,1,null) over (order by f) +from (select * from nulltable union all select * from nulltable) x; +---- TYPES +STRING, STRING +---- RESULTS +'a\x00b','' +'a\x00b','NULL' +====
