You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2017/04/28 17:35:09 UTC

[Impala-ASF-CR] IMPALA-5246: UDF's Close() should handle Expr's preparation failure 

Michael Ho has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/6757

Change subject: IMPALA-5246: UDF's Close() should handle Expr's preparation failure
......................................................................

IMPALA-5246: UDF's Close() should handle Expr's preparation failure

UDF may fail to initialize due low memory limit or
other reasons In which case, its Prepare() function may
not have been called and its thread local state may not be
initialized.

The Close() functions of some of the built-in and test-udf
made the wrong assumption that the thread local states are
always initialized. This may lead to de-referencing null
pointer in Close(). This change fixes this issue by
checking the thread local state is not null and returns
early if so.

Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
---
M be/src/exprs/case-expr.cc
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/utility-functions.cc
M be/src/testutil/test-udfs.cc
6 files changed, 17 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/6757/1
-- 
To view, visit http://gerrit.cloudera.org:8080/6757
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>

[Impala-ASF-CR] IMPALA-5246: MemTestClose() should hanle Expr's preparation failure

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Taras Bobrovytsky,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6757

to look at the new patch set (#3).

Change subject: IMPALA-5246: MemTestClose() should hanle Expr's preparation failure
......................................................................

IMPALA-5246: MemTestClose() should hanle Expr's preparation failure

UDF may fail to initialize due exceeding memory limit or
other reasons. In which case, its Prepare() function may
not have been called and its thread local state may not be
initialized.

MemTestClose() in test-udf.cc made the wrong assumption that
the thread local states are always initialized. This may lead
to de-referencing null pointer in Close(). This change fixes
this issue by checking the thread local state is not null and
returns early if so. Also sets the fragment or thread local
states in FunctionContext to nullptr after freeing them in
various built-in's Close() functions.

Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
---
M be/src/exprs/case-expr.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/in-predicate.h
M be/src/exprs/like-predicate.cc
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/utility-functions.cc
M be/src/testutil/test-udfs.cc
M be/src/udf/udf-test.cc
11 files changed, 25 insertions(+), 18 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/6757/3
-- 
To view, visit http://gerrit.cloudera.org:8080/6757
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-5246: MemTestClose() should handle Expr's preparation failure

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5246: MemTestClose() should handle Expr's preparation failure
......................................................................


Patch Set 6:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/534/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5246: MemTestClose() should handle Expr's preparation failure

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Taras Bobrovytsky,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6757

to look at the new patch set (#5).

Change subject: IMPALA-5246: MemTestClose() should handle Expr's preparation failure
......................................................................

IMPALA-5246: MemTestClose() should handle Expr's preparation failure

UDF may fail to initialize due exceeding memory limit or
other reasons. In which case, its Prepare() function may
not have been called and its thread local state may not be
initialized.

MemTestClose() in test-udf.cc made the wrong assumption that
the thread local states are always initialized. This may lead
to de-referencing null pointer in Close(). This change fixes
this issue by checking the thread local state is not null and
returns early if so. Also sets the fragment or thread local
states in FunctionContext to nullptr after freeing them in
various built-in's Close() functions.

Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
---
M be/src/exprs/case-expr.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/in-predicate.h
M be/src/exprs/like-predicate.cc
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/utility-functions.cc
M be/src/testutil/test-udfs.cc
M be/src/udf/udf-test.cc
11 files changed, 28 insertions(+), 20 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/6757/5
-- 
To view, visit http://gerrit.cloudera.org:8080/6757
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-5246: MemTestClose() should handle Expr's preparation failure

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Taras Bobrovytsky,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6757

to look at the new patch set (#6).

Change subject: IMPALA-5246: MemTestClose() should handle Expr's preparation failure
......................................................................

IMPALA-5246: MemTestClose() should handle Expr's preparation failure

UDF may fail to initialize due exceeding memory limit or
other reasons. In which case, its Prepare() function may
not have been called and its thread local state may not be
initialized.

MemTestClose() in test-udf.cc made the wrong assumption that
the thread local states are always initialized. This may lead
to de-referencing null pointer in Close(). This change fixes
this issue by checking the thread local state is not null and
returns early if so. Also sets the fragment or thread local
states in FunctionContext to nullptr after freeing them in
various built-in's Close() functions.

Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
---
M be/src/exprs/case-expr.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/in-predicate.h
M be/src/exprs/like-predicate.cc
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/utility-functions.cc
M be/src/testutil/test-udfs.cc
M be/src/udf/udf-test.cc
11 files changed, 28 insertions(+), 20 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/6757/6
-- 
To view, visit http://gerrit.cloudera.org:8080/6757
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-5246: MemTestClose() should handle Expr's preparation failure

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.

Change subject: IMPALA-5246: MemTestClose() should handle Expr's preparation failure
......................................................................


Patch Set 5:

(6 comments)

The existing code wasn't quite consistent about checking for null before calling FunctionContextImpl::Free() (e.g. UdfBuiltins::TruncClose()). The new patch now removes all null check before calling FunctionContextImpl::Close() and delete. Also set the FunctionContext states to nullptr after freeing them.

I did a quick audit of all the Close() functions in our built-ins and didn't see any potential nullptr dereference.

http://gerrit.cloudera.org:8080/#/c/6757/1//COMMIT_MSG
Commit Message:

PS1, Line 10: .
> Put a period here
Done


http://gerrit.cloudera.org:8080/#/c/6757/1/be/src/exprs/case-expr.cc
File be/src/exprs/case-expr.cc:

PS1, Line 82: st<uint8_t*>(case_state));
> I could be wrong, but shouldn't we call
Close() is supposed to be called once per ExprContext so the code is not supposed to be calling fn_ctx->Free() on the same FunctionContext twice. Moreover, it's not entirely clear to me Close() is really idempotent (e.g. calling close_fn_() on ScalarFnCall() may have unexpected side effect).

That said, it still seems like a good practice to set the THREAD_LOCAL_STATE to nullptr after freeing it. Thanks for the suggestion.


http://gerrit.cloudera.org:8080/#/c/6757/1/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

PS1, Line 182: 
> same here
Done


http://gerrit.cloudera.org:8080/#/c/6757/1/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:

PS1, Line 614: terpret_ca
> same here
Done


http://gerrit.cloudera.org:8080/#/c/6757/1/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:

PS1, Line 195: 
> same here
Done


http://gerrit.cloudera.org:8080/#/c/6757/1/be/src/exprs/utility-functions.cc
File be/src/exprs/utility-functions.cc:

PS1, Line 59: 
> same here
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5246: UDF's Close() should handle Expr's preparation failure

Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-5246: UDF's Close() should handle Expr's preparation failure
......................................................................


Patch Set 1: Code-Review+1

(1 comment)

This change makes sense to me.

http://gerrit.cloudera.org:8080/#/c/6757/1//COMMIT_MSG
Commit Message:

PS1, Line 10:  
Put a period here


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5246: MemTestClose() should handle Expr's preparation failure

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Taras Bobrovytsky,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6757

to look at the new patch set (#4).

Change subject: IMPALA-5246: MemTestClose() should handle Expr's preparation failure
......................................................................

IMPALA-5246: MemTestClose() should handle Expr's preparation failure

UDF may fail to initialize due exceeding memory limit or
other reasons. In which case, its Prepare() function may
not have been called and its thread local state may not be
initialized.

MemTestClose() in test-udf.cc made the wrong assumption that
the thread local states are always initialized. This may lead
to de-referencing null pointer in Close(). This change fixes
this issue by checking the thread local state is not null and
returns early if so. Also sets the fragment or thread local
states in FunctionContext to nullptr after freeing them in
various built-in's Close() functions.

Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
---
M be/src/exprs/case-expr.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/in-predicate.h
M be/src/exprs/like-predicate.cc
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/utility-functions.cc
M be/src/testutil/test-udfs.cc
M be/src/udf/udf-test.cc
11 files changed, 25 insertions(+), 18 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/6757/4
-- 
To view, visit http://gerrit.cloudera.org:8080/6757
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-5246: MemTestClose() should handle Expr's preparation failure

Posted by "Attila Jeges (Code Review)" <ge...@cloudera.org>.
Attila Jeges has posted comments on this change.

Change subject: IMPALA-5246: MemTestClose() should handle Expr's preparation failure
......................................................................


Patch Set 6: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5246: UDF's Close() should handle Expr's preparation failure

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Taras Bobrovytsky,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6757

to look at the new patch set (#2).

Change subject: IMPALA-5246: UDF's Close() should handle Expr's preparation failure
......................................................................

IMPALA-5246: UDF's Close() should handle Expr's preparation failure

UDF may fail to initialize due exceeding memory limit or
other reasons. In which case, its Prepare() function may
not have been called and its thread local state may not be
initialized.

MemTestClose() in test-udf.cc made the wrong assumption that
the thread local states are always initialized. This may lead
to de-referencing null pointer in Close(). This change fixes
this issue by checking the thread local state is not null and
returns early if so. Also sets the fragment or thread local
states in FunctionContext to nullptr after freeing them in
various built-in's Close() functions.

Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
---
M be/src/exprs/case-expr.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/in-predicate.h
M be/src/exprs/like-predicate.cc
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/utility-functions.cc
M be/src/testutil/test-udfs.cc
M be/src/udf/udf-test.cc
11 files changed, 25 insertions(+), 18 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/6757/2
-- 
To view, visit http://gerrit.cloudera.org:8080/6757
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-5246: UDF's Close() should handle Expr's preparation failure

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-5246: UDF's Close() should handle Expr's preparation failure
......................................................................


Patch Set 1:

Free() already has a null check. What am I missing?

void FunctionContext::Free(uint8_t* buffer) noexcept {
  assert(!impl_->closed_);
  if (buffer == NULL) return;

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5246: MemTestClose() should handle Expr's preparation failure

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5246: MemTestClose() should handle Expr's preparation failure
......................................................................


IMPALA-5246: MemTestClose() should handle Expr's preparation failure

UDF may fail to initialize due exceeding memory limit or
other reasons. In which case, its Prepare() function may
not have been called and its thread local state may not be
initialized.

MemTestClose() in test-udf.cc made the wrong assumption that
the thread local states are always initialized. This may lead
to de-referencing null pointer in Close(). This change fixes
this issue by checking the thread local state is not null and
returns early if so. Also sets the fragment or thread local
states in FunctionContext to nullptr after freeing them in
various built-in's Close() functions.

Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
Reviewed-on: http://gerrit.cloudera.org:8080/6757
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Reviewed-by: Attila Jeges <at...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/exprs/case-expr.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/in-predicate.h
M be/src/exprs/like-predicate.cc
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/utility-functions.cc
M be/src/testutil/test-udfs.cc
M be/src/udf/udf-test.cc
11 files changed, 28 insertions(+), 20 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Attila Jeges: Looks good to me, but someone else must approve
  Dan Hecht: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-5246: MemTestClose() should handle Expr's preparation failure

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5246: MemTestClose() should handle Expr's preparation failure
......................................................................


Patch Set 6: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5246: MemTestClose() should handle Expr's preparation failure

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-5246: MemTestClose() should handle Expr's preparation failure
......................................................................


Patch Set 6: Code-Review+2

This version looks okay to me but please give other reviews a chance to look as well.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5246: UDF's Close() should handle Expr's preparation failure

Posted by "Attila Jeges (Code Review)" <ge...@cloudera.org>.
Attila Jeges has posted comments on this change.

Change subject: IMPALA-5246: UDF's Close() should handle Expr's preparation failure
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/6757/1/be/src/exprs/case-expr.cc
File be/src/exprs/case-expr.cc:

PS1, Line 82: fn_ctx->Free(reinterpret_cast<uint8_t*>(case_state))
I could be wrong, but shouldn't we call
fn_ctx->SetFunctionState(FunctionContext::THREAD_LOCAL, nullptr)
after fn_ctx->Free(case_state) to avoid double-free problems ?
(just like we do in Close functions in be/src/testutil/test-udfs.cc)


http://gerrit.cloudera.org:8080/#/c/6757/1/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

PS1, Line 182: ctx->Free(seed);
same here


http://gerrit.cloudera.org:8080/#/c/6757/1/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:

PS1, Line 614: delete re;
same here


http://gerrit.cloudera.org:8080/#/c/6757/1/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:

PS1, Line 195: delete dt_ctx
same here


http://gerrit.cloudera.org:8080/#/c/6757/1/be/src/exprs/utility-functions.cc
File be/src/exprs/utility-functions.cc:

PS1, Line 59: delete uuid_gen;
same here


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes