impala-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From taras...@apache.org
Subject [13/18] incubator-impala git commit: IMPALA-3018: Don't return NULL on zero length allocations.
Date Thu, 14 Jul 2016 19:05:10 GMT
IMPALA-3018: Don't return NULL on zero length allocations.

FunctionContext::Allocate() and FunctionContext::AllocateLocal()
used to return NULL for zero length allocations. This makes
it hard to distinguish between allocation failures and zero
length allocations. Such confusion may lead to DCHECK failure
in the macro RETURN_IF_NULL() in debug builds or access to NULL
pointers in non-debug builds.

This change fixes the problem above by returning NULL only if
there is allocation failure. Zero-length allocations will always
return a dummy non-NULL pointer.

Change-Id: Id8c3211f4d9417f44b8018ccc58ae182682693da
Reviewed-on: http://gerrit.cloudera.org:8080/3601
Reviewed-by: Michael Ho <kwho@cloudera.com>
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/f129dfd2
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/f129dfd2
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/f129dfd2

Branch: refs/heads/master
Commit: f129dfd2022d268a5658bac9da279df79dd6c857
Parents: 900f148
Author: Michael Ho <kwho@cloudera.com>
Authored: Fri Jul 8 14:39:44 2016 -0700
Committer: Taras Bobrovytsky <tarasbob@apache.org>
Committed: Thu Jul 14 19:04:45 2016 +0000

----------------------------------------------------------------------
 be/src/exprs/aggregate-functions-ir.cc          |  1 +
 be/src/runtime/free-pool.h                      |  8 +++----
 be/src/udf/udf-test.cc                          |  5 ++--
 be/src/udf/udf.cc                               |  2 --
 .../queries/QueryTest/aggregation.test          | 24 +++++++++++++++++---
 5 files changed, 28 insertions(+), 12 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f129dfd2/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 79d7ac7..853661c 100644
--- a/be/src/exprs/aggregate-functions-ir.cc
+++ b/be/src/exprs/aggregate-functions-ir.cc
@@ -1388,6 +1388,7 @@ void AggregateFunctions::LastValUpdate(FunctionContext* ctx, const StringVal&
sr
   } else {
     new_ptr = ctx->Reallocate(dst->ptr, src.len);
   }
+  // Note that a zero-length string is not the same as StringVal::null().
   RETURN_IF_NULL(ctx, new_ptr);
   dst->ptr = new_ptr;
   memcpy(dst->ptr, src.ptr, src.len);

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f129dfd2/be/src/runtime/free-pool.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/free-pool.h b/be/src/runtime/free-pool.h
index 90df749..8762f1d 100644
--- a/be/src/runtime/free-pool.h
+++ b/be/src/runtime/free-pool.h
@@ -62,12 +62,10 @@ class FreePool {
       return NULL;
     }
 #endif
-    ++net_allocations_;
-    if (FLAGS_disable_mem_pools) return reinterpret_cast<uint8_t*>(malloc(size));
-
     /// Return a non-NULL dummy pointer. NULL is reserved for failures.
     if (UNLIKELY(size == 0)) return mem_pool_->EmptyAllocPtr();
-
+    ++net_allocations_;
+    if (FLAGS_disable_mem_pools) return reinterpret_cast<uint8_t*>(malloc(size));
     int free_list_idx = Bits::Log2Ceiling64(size);
     DCHECK_LT(free_list_idx, NUM_LISTS);
     FreeListNode* allocation = lists_[free_list_idx].next;
@@ -92,12 +90,12 @@ class FreePool {
   }
 
   void Free(uint8_t* ptr) {
+    if (UNLIKELY(ptr == NULL || ptr == mem_pool_->EmptyAllocPtr())) return;
     --net_allocations_;
     if (FLAGS_disable_mem_pools) {
       free(ptr);
       return;
     }
-    if (UNLIKELY(ptr == NULL || ptr == mem_pool_->EmptyAllocPtr())) return;
     FreeListNode* node = reinterpret_cast<FreeListNode*>(ptr - sizeof(FreeListNode));
     FreeListNode* list = node->list;
 #ifndef NDEBUG

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f129dfd2/be/src/udf/udf-test.cc
----------------------------------------------------------------------
diff --git a/be/src/udf/udf-test.cc b/be/src/udf/udf-test.cc
index 4857a06..86f02c2 100644
--- a/be/src/udf/udf-test.cc
+++ b/be/src/udf/udf-test.cc
@@ -123,8 +123,9 @@ IntVal ValidateFail(FunctionContext* context) {
 }
 
 IntVal ValidateMem(FunctionContext* context) {
-  EXPECT_TRUE(context->Allocate(0) == NULL);
-  uint8_t* buffer = context->Allocate(10);
+  uint8_t* buffer = context->Allocate(0);
+  EXPECT_TRUE(buffer != NULL);
+  buffer = context->Reallocate(buffer, 10);
   EXPECT_TRUE(buffer != NULL);
   memset(buffer, 0, 10);
   context->Free(buffer);

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f129dfd2/be/src/udf/udf.cc
----------------------------------------------------------------------
diff --git a/be/src/udf/udf.cc b/be/src/udf/udf.cc
index ff81c9c..c1fb5be 100644
--- a/be/src/udf/udf.cc
+++ b/be/src/udf/udf.cc
@@ -286,7 +286,6 @@ inline bool FunctionContextImpl::CheckAllocResult(const char* fn_name,
 
 uint8_t* FunctionContext::Allocate(int byte_size) noexcept {
   assert(!impl_->closed_);
-  if (byte_size == 0) return NULL;
   uint8_t* buffer = impl_->pool_->Allocate(byte_size);
   if (UNLIKELY(!impl_->CheckAllocResult("FunctionContext::Allocate",
       buffer, byte_size))) {
@@ -418,7 +417,6 @@ void FunctionContext::SetFunctionState(FunctionStateScope scope, void*
ptr) {
 
 uint8_t* FunctionContextImpl::AllocateLocal(int64_t byte_size) noexcept {
   assert(!closed_);
-  if (byte_size == 0) return NULL;
   uint8_t* buffer = pool_->Allocate(byte_size);
   if (UNLIKELY(!CheckAllocResult("FunctionContextImpl::AllocateLocal",
       buffer, byte_size))) {

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f129dfd2/testdata/workloads/functional-query/queries/QueryTest/aggregation.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/aggregation.test b/testdata/workloads/functional-query/queries/QueryTest/aggregation.test
index 5389b04..740f777 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/aggregation.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/aggregation.test
@@ -154,13 +154,31 @@ from alltypesagg where day is not null
 bigint, string, string, string, string
 ====
 ---- QUERY
-# Test for IMPALA-3018. Verify update() function of min() handles
+# Test for IMPALA-3018. Verify update() functions of min() and max() handle
 # zero-length string correctly.
-select min(str) from (values ('aaa' as str), ('')) as tmp
+select max(str), min(str) from (values ('aaa' as str), (''), ('123')) as tmp
+---- RESULTS
+'aaa',''
+---- TYPES
+string,string
+====
+---- QUERY
+# Test for IMPALA-3018. Verify update() function of last_value() handles
+# zero-length string correctly.
+select last_value(b) over (partition by a order by d) from functional.nulltable;
 ---- RESULTS
 ''
 ---- TYPES
-String
+string
+====
+---- QUERY
+# Test for IMPALA-3018. Verify update() function of first_value() handles
+# zero-length string correctly.
+select first_value(b) over (partition by a order by d) from functional.nulltable;
+---- RESULTS
+''
+---- TYPES
+string
 ====
 ---- QUERY
 # grouping by different data types, with NULLs


Mime
View raw message