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/11/16 07:43:02 UTC

[Impala-ASF-CR] IMPALA-6184: Clean up aftr ScalarExprEvaluator::Clone() fails

Michael Ho has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8572


Change subject: IMPALA-6184: Clean up aftr ScalarExprEvaluator::Clone() fails
......................................................................

IMPALA-6184: Clean up aftr ScalarExprEvaluator::Clone() fails

When ScalarExprEvaluator::Clone() fails, the newly created evaluator was
not added to the output vector. This makes it impossible for callers to
close and clean up the evaluators afterwards. This change fixes this by
always adding the newly created evaluator to the output vector before
checking for the error status.

This path is only exercised in the scanner code. Two new tests are added
to exercise the failure paths.

Testing done: newly added tests in udf-errors.test

Change-Id: I45ffd722d0a69ad05ae3c748cf504c7f1a959a1d
---
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/testutil/test-udfs.cc
M testdata/workloads/functional-query/queries/QueryTest/udf-errors.test
3 files changed, 88 insertions(+), 2 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-6184: Clean up aftr ScalarExprEvaluator::Clone() fails

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8572 )

Change subject: IMPALA-6184: Clean up aftr ScalarExprEvaluator::Clone() fails
......................................................................


Patch Set 1:

(2 comments)

Just a couple of questions.

http://gerrit.cloudera.org:8080/#/c/8572/1/be/src/exprs/scalar-expr-evaluator.cc
File be/src/exprs/scalar-expr-evaluator.cc:

http://gerrit.cloudera.org:8080/#/c/8572/1/be/src/exprs/scalar-expr-evaluator.cc@193
PS1, Line 193:     Status status =
Kinda subtle, can you add a comment like the one above?

    // Always add the evaluator to the vector so it can be cleaned up.


http://gerrit.cloudera.org:8080/#/c/8572/1/testdata/workloads/functional-query/queries/QueryTest/udf-errors.test
File testdata/workloads/functional-query/queries/QueryTest/udf-errors.test:

http://gerrit.cloudera.org:8080/#/c/8572/1/testdata/workloads/functional-query/queries/QueryTest/udf-errors.test@85
PS1, Line 85: on (bad_expr2(rand()) = (t2.bool_col && t1.bool_col));
Is this one guaranteed to fail? It seems like if we're unlucky, rand() could not return one of the values that BadExpr fails on.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I45ffd722d0a69ad05ae3c748cf504c7f1a959a1d
Gerrit-Change-Number: 8572
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 15:50:19 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6184: Clean up after ScalarExprEvaluator::Clone() fails

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

Hello Tim Armstrong, 

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

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

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

Change subject: IMPALA-6184: Clean up after ScalarExprEvaluator::Clone() fails
......................................................................

IMPALA-6184: Clean up after ScalarExprEvaluator::Clone() fails

When ScalarExprEvaluator::Clone() fails, the newly created evaluator was
not added to the output vector. This makes it impossible for callers to
close and clean up the evaluators afterwards. This change fixes this by
always adding the newly created evaluator to the output vector before
checking for the error status.

This path is only exercised in the scanner code. Two new tests are added
to exercise the failure paths.

Testing done: newly added tests in udf-errors.test

Change-Id: I45ffd722d0a69ad05ae3c748cf504c7f1a959a1d
---
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/testutil/test-udfs.cc
M testdata/workloads/functional-query/queries/QueryTest/udf-errors.test
3 files changed, 89 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I45ffd722d0a69ad05ae3c748cf504c7f1a959a1d
Gerrit-Change-Number: 8572
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-6184: Clean up after ScalarExprEvaluator::Clone() fails

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8572 )

Change subject: IMPALA-6184: Clean up after ScalarExprEvaluator::Clone() fails
......................................................................

IMPALA-6184: Clean up after ScalarExprEvaluator::Clone() fails

When ScalarExprEvaluator::Clone() fails, the newly created evaluator was
not added to the output vector. This makes it impossible for callers to
close and clean up the evaluators afterwards. This change fixes this by
always adding the newly created evaluator to the output vector before
checking for the error status.

This path is only exercised in the scanner code. Two new tests are added
to exercise the failure paths.

Testing done: newly added tests in udf-errors.test

Change-Id: I45ffd722d0a69ad05ae3c748cf504c7f1a959a1d
Reviewed-on: http://gerrit.cloudera.org:8080/8572
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/testutil/test-udfs.cc
M testdata/workloads/functional-query/queries/QueryTest/udf-errors.test
3 files changed, 89 insertions(+), 2 deletions(-)

Approvals:
  Tim Armstrong: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I45ffd722d0a69ad05ae3c748cf504c7f1a959a1d
Gerrit-Change-Number: 8572
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-6184: Clean up after ScalarExprEvaluator::Clone() fails

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

Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8572 )

Change subject: IMPALA-6184: Clean up after ScalarExprEvaluator::Clone() fails
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8572/1/be/src/exprs/scalar-expr-evaluator.cc
File be/src/exprs/scalar-expr-evaluator.cc:

http://gerrit.cloudera.org:8080/#/c/8572/1/be/src/exprs/scalar-expr-evaluator.cc@193
PS1, Line 193:     Status status =
> Kinda subtle, can you add a comment like the one above?
Done


http://gerrit.cloudera.org:8080/#/c/8572/1/testdata/workloads/functional-query/queries/QueryTest/udf-errors.test
File testdata/workloads/functional-query/queries/QueryTest/udf-errors.test:

http://gerrit.cloudera.org:8080/#/c/8572/1/testdata/workloads/functional-query/queries/QueryTest/udf-errors.test@85
PS1, Line 85: on (bad_expr2(rand()) = (t2.bool_col && t1.bool_col));
> Is this one guaranteed to fail? It seems like if we're unlucky, rand() coul
PS2 has been updated to fail more deterministically. rand() with the same seed should generate the same sequence of numbers so in theory it should work but let's make things simpler. I added rand() to avoid the FE from mucking with the expression due to constant folding.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I45ffd722d0a69ad05ae3c748cf504c7f1a959a1d
Gerrit-Change-Number: 8572
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 19:55:05 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6184: Clean up after ScalarExprEvaluator::Clone() fails

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8572 )

Change subject: IMPALA-6184: Clean up after ScalarExprEvaluator::Clone() fails
......................................................................


Patch Set 2:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1490/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I45ffd722d0a69ad05ae3c748cf504c7f1a959a1d
Gerrit-Change-Number: 8572
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Nov 2017 23:01:33 +0000
Gerrit-HasComments: No