You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Dan Hecht (Code Review)" <ge...@cloudera.org> on 2017/04/28 20:41:58 UTC
[Impala-ASF-CR] IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem limit exceeded
Dan Hecht has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6761
Change subject: IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
......................................................................
IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
We need to check for AllocateLocal() returning NULL. CopyFrom() takes
care of that for us. Also adjust a few other places in the code base
that didn't have the check.
The new test reproduces the crash, but in order to get this test file to
execute, I had to move the xfail to be a function marker. Apparently
xfail as a statement causes the test to not run at all. We should run
all of these queries even if they are non-determistic to at least verify
that impalad does not crash.
Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
---
M be/src/exprs/hive-udf-call.cc
M be/src/udf/uda-test.cc
M be/src/udf/udf-test.cc
M be/src/udf_samples/uda-sample.cc
M testdata/workloads/functional-query/queries/QueryTest/alloc-fail-update.test
M tests/custom_cluster/test_alloc_fail.py
6 files changed, 23 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/6761/1
--
To view, visit http://gerrit.cloudera.org:8080/6761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
[Impala-ASF-CR] IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem limit exceeded
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6761/3/be/src/exprs/aggregate-functions-ir.cc
File be/src/exprs/aggregate-functions-ir.cc:
PS3, Line 1295: reinterpret_cast<const uint8_t*>
oops, i had to add this cast. Michael do you still prefer to call CopyFrom here?
http://gerrit.cloudera.org:8080/#/c/6761/3/be/src/exprs/udf-builtins-ir.cc
File be/src/exprs/udf-builtins-ir.cc:
PS3, Line 512: reinterpret_cast<const uint8_t*>(str.c_str())
and here.
--
To view, visit http://gerrit.cloudera.org:8080/6761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem limit exceeded
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem limit exceeded
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem limit exceeded
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/6761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem limit exceeded
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6761/1//COMMIT_MSG
Commit Message:
PS1, Line 10: a few other places
Thanks for doing it. Please feel free to ignore but there are a few places in the code which can be converted to using StringVal::CopyFrom() even though they have the check:
AggregateFunctions::ReservoirSampleFinalize(), CastFunctions::CastToChar(), udf-builtins-ir.cc, ?
--
To view, visit http://gerrit.cloudera.org:8080/6761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem limit exceeded
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Matthew Jacobs,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6761
to look at the new patch set (#3).
Change subject: IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
......................................................................
IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
We need to check for AllocateLocal() returning NULL. CopyFrom() takes
care of that for us. Also adjust a few other places in the code base
that didn't have the check.
The new test reproduces the crash, but in order to get this test file to
execute, I had to move the xfail to be a function decorator. Apparently
xfail as a statement causes the test to not run at all. We should run
all of these queries even if they are non-determistic to at least verify
that impalad does not crash.
Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
---
M be/src/exprs/aggregate-functions-ir.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/udf/uda-test.cc
M be/src/udf/udf-test.cc
M be/src/udf_samples/uda-sample.cc
M testdata/workloads/functional-query/queries/QueryTest/alloc-fail-update.test
M tests/custom_cluster/test_alloc_fail.py
8 files changed, 27 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/6761/3
--
To view, visit http://gerrit.cloudera.org:8080/6761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem limit exceeded
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
......................................................................
IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
We need to check for AllocateLocal() returning NULL. CopyFrom() takes
care of that for us. Also adjust a few other places in the code base
that didn't have the check.
The new test reproduces the crash, but in order to get this test file to
execute, I had to move the xfail to be a function decorator. Apparently
xfail as a statement causes the test to not run at all. We should run
all of these queries even if they are non-determistic to at least verify
that impalad does not crash.
Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
Reviewed-on: http://gerrit.cloudera.org:8080/6761
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/exprs/aggregate-functions-ir.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/udf/uda-test.cc
M be/src/udf/udf-test.cc
M be/src/udf_samples/uda-sample.cc
M testdata/workloads/functional-query/queries/QueryTest/alloc-fail-update.test
M tests/custom_cluster/test_alloc_fail.py
8 files changed, 27 insertions(+), 25 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Dan Hecht: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/6761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem limit exceeded
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6761/1/be/src/udf_samples/uda-sample.cc
File be/src/udf_samples/uda-sample.cc:
Line 86: void StringConcatUpdate(FunctionContext* context, const StringVal& arg1,
> How do make changes to that?
I think you should have access to push to that repo. Github also has an editor so you can click on the edit icon directly on the page and make edits, then commit it. We should kill this repo soon though this is clearly burdensome.
--
To view, visit http://gerrit.cloudera.org:8080/6761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem limit exceeded
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
......................................................................
Patch Set 4:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/517/
--
To view, visit http://gerrit.cloudera.org:8080/6761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem limit exceeded
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
......................................................................
Patch Set 1: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6761/1//COMMIT_MSG
Commit Message:
PS1, Line 14: marker
I believe in python this is called a decorator.
http://gerrit.cloudera.org:8080/#/c/6761/1/be/src/udf_samples/uda-sample.cc
File be/src/udf_samples/uda-sample.cc:
Line 86: void StringConcatUpdate(FunctionContext* context, const StringVal& arg1,
until we remove the impala-udf-sample repo, we should probably keep that code up to date as well:
https://github.com/cloudera/impala-udf-samples/blob/master/uda-sample.cc
--
To view, visit http://gerrit.cloudera.org:8080/6761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem limit exceeded
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6761/1//COMMIT_MSG
Commit Message:
PS1, Line 10: a few other places
> Thanks for doing it. Please feel free to ignore but there are a few places
I did the ones where we can get rid of the is_null check at the callsite. For some of them, we still need the is_null check, so it seems clearest to not use CopyFrom() in those cases.
PS1, Line 14: marker
> I believe in python this is called a decorator.
Done
http://gerrit.cloudera.org:8080/#/c/6761/1/be/src/udf_samples/uda-sample.cc
File be/src/udf_samples/uda-sample.cc:
Line 86: void StringConcatUpdate(FunctionContext* context, const StringVal& arg1,
> until we remove the impala-udf-sample repo, we should probably keep that co
How do make changes to that?
--
To view, visit http://gerrit.cloudera.org:8080/6761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem limit exceeded
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6761/3/be/src/exprs/aggregate-functions-ir.cc
File be/src/exprs/aggregate-functions-ir.cc:
PS3, Line 1295: reinterpret_cast<const uint8_t*>
> oops, i had to add this cast. Michael do you still prefer to call CopyFrom
Hmm...it still looks cleaner to call CopyFrom() if possible.
--
To view, visit http://gerrit.cloudera.org:8080/6761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem limit exceeded
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem limit exceeded
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Matthew Jacobs,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6761
to look at the new patch set (#2).
Change subject: IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
......................................................................
IMPALA-5252: Fix crash in HiveUdfCall::GetStringVal() when mem_limit exceeded
We need to check for AllocateLocal() returning NULL. CopyFrom() takes
care of that for us. Also adjust a few other places in the code base
that didn't have the check.
The new test reproduces the crash, but in order to get this test file to
execute, I had to move the xfail to be a function decorator. Apparently
xfail as a statement causes the test to not run at all. We should run
all of these queries even if they are non-determistic to at least verify
that impalad does not crash.
Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
---
M be/src/exprs/aggregate-functions-ir.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/udf/uda-test.cc
M be/src/udf/udf-test.cc
M be/src/udf_samples/uda-sample.cc
M testdata/workloads/functional-query/queries/QueryTest/alloc-fail-update.test
M tests/custom_cluster/test_alloc_fail.py
8 files changed, 25 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/6761/2
--
To view, visit http://gerrit.cloudera.org:8080/6761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iafefef24479164cc4d2b99191d2de28eb8b311b6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>