You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Alex Behm (Code Review)" <ge...@cloudera.org> on 2016/09/09 00:55:31 UTC

[Impala-ASF-CR] IMPALA-4098: Open()/Close() partition exprs once per fragment instance. 

Alex Behm has uploaded a new change for review.

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

Change subject: IMPALA-4098: Open()/Close() partition exprs once per fragment instance.
......................................................................

IMPALA-4098: Open()/Close() partition exprs once per fragment instance.

Partition exprs stored in the descriptor table can be referenced by multiple
exec nodes (and/or a data sink) within the same fragment, so the lifecycle
of those exprs (Prepare/Open/Close) is tied to the fragment and not to
a particular exec node.

A recent change exposed this improper lifecycle management because we cloned
the partition exprs before using them, but by that time the exprs had been
closed which caused the cloning function to hit a DCHECK.

The fix is to tie the lifecycle of those exprs to that of the fragment
instance.

Testing: I could reliably reproduce the bug by running this query in a loop:

set num_nodes=1;
select count(a.year), count(a.month), count(a.int_col),
       count(b.year), count(b.month), count(b.int_col)
from functional.alltypessmall a, functional.alltypessmall b;

After this patch I was not able to reproduce the bug anymore. I don't think
it makes sense to add a test specifically for this bug because our existing
tests already caught it, and the hit DCHECK does not exist anymore due to
restructuring.

Change-Id: Id179df645e500530f4418988f6ce64a03d669892
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-table-sink.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M be/src/runtime/plan-fragment-executor.cc
6 files changed, 57 insertions(+), 106 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id179df645e500530f4418988f6ce64a03d669892
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>

[Impala-ASF-CR] IMPALA-4098: Open()/Close() partition exprs once per fragment instance.

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

Change subject: IMPALA-4098: Open()/Close() partition exprs once per fragment instance.
......................................................................


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id179df645e500530f4418988f6ce64a03d669892
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4098: Open()/Close() partition exprs once per fragment instance.

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Hello Marcel Kornacker, Tim Armstrong,

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

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

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

Change subject: IMPALA-4098: Open()/Close() partition exprs once per fragment instance.
......................................................................

IMPALA-4098: Open()/Close() partition exprs once per fragment instance.

Partition exprs stored in the descriptor table can be referenced by multiple
exec nodes (and/or a data sink) within the same fragment instance, so the
lifecycle of those exprs (Prepare/Open/Close) is tied to the fragment instance
and not to a particular exec node.

A recent change exposed this improper lifecycle management because we cloned
the partition exprs before using them, but by that time the exprs had been
closed which caused the cloning function to hit a DCHECK.

The fix is to tie the lifecycle of those exprs to that of the fragment
instance.

Testing: I could reliably reproduce the bug by running this query in a loop:

set num_nodes=1;
select count(a.year), count(a.month), count(a.int_col),
       count(b.year), count(b.month), count(b.int_col)
from functional.alltypessmall a, functional.alltypessmall b;

After this patch I was not able to reproduce the bug anymore. I don't think
it makes sense to add a test specifically for this bug because our existing
tests already caught it, and the hit DCHECK does not exist anymore due to
restructuring.

Change-Id: Id179df645e500530f4418988f6ce64a03d669892
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-table-sink.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M be/src/runtime/plan-fragment-executor.cc
6 files changed, 59 insertions(+), 106 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id179df645e500530f4418988f6ce64a03d669892
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4098: Open()/Close() partition exprs once per fragment instance.

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

Change subject: IMPALA-4098: Open()/Close() partition exprs once per fragment instance.
......................................................................


Patch Set 2: Code-Review+2

(2 comments)

so much simpler.

http://gerrit.cloudera.org:8080/#/c/4340/2//COMMIT_MSG
Commit Message:

Line 10: exec nodes (and/or a data sink) within the same fragment, so the lifecycle
do you mean fragment or fragment instance? please clarify. (and if you really mean fragment, explain.)


http://gerrit.cloudera.org:8080/#/c/4340/2/be/src/runtime/descriptors.h
File be/src/runtime/descriptors.h:

Line 438:   Status PrepareAndOpenExprs(RuntimeState* state) const;
..PartitionExprs()? (or do you anticipate throwing more exprs into this?)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id179df645e500530f4418988f6ce64a03d669892
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4098: Open()/Close() partition exprs once per fragment instance.

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

Change subject: IMPALA-4098: Open()/Close() partition exprs once per fragment instance.
......................................................................


IMPALA-4098: Open()/Close() partition exprs once per fragment instance.

Partition exprs stored in the descriptor table can be referenced by multiple
exec nodes (and/or a data sink) within the same fragment instance, so the
lifecycle of those exprs (Prepare/Open/Close) is tied to the fragment instance
and not to a particular exec node.

A recent change exposed this improper lifecycle management because we cloned
the partition exprs before using them, but by that time the exprs had been
closed which caused the cloning function to hit a DCHECK.

The fix is to tie the lifecycle of those exprs to that of the fragment
instance.

Testing: I could reliably reproduce the bug by running this query in a loop:

set num_nodes=1;
select count(a.year), count(a.month), count(a.int_col),
       count(b.year), count(b.month), count(b.int_col)
from functional.alltypessmall a, functional.alltypessmall b;

After this patch I was not able to reproduce the bug anymore. I don't think
it makes sense to add a test specifically for this bug because our existing
tests already caught it, and the hit DCHECK does not exist anymore due to
restructuring.

Change-Id: Id179df645e500530f4418988f6ce64a03d669892
Reviewed-on: http://gerrit.cloudera.org:8080/4340
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-table-sink.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M be/src/runtime/plan-fragment-executor.cc
6 files changed, 59 insertions(+), 106 deletions(-)

Approvals:
  Internal Jenkins: Verified
  Alex Behm: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Id179df645e500530f4418988f6ce64a03d669892
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4098: Open()/Close() partition exprs once per fragment instance.

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

Change subject: IMPALA-4098: Open()/Close() partition exprs once per fragment instance.
......................................................................


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/4340/1/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:

Line 216:   for (const auto& entry: tdesc.hdfsTable.partitions) {
> clang-format puts a space before :.
Done


Line 512:     for (const auto& part_entry: hdfs_tbl->partition_descriptors()) {
> space before : to avoid later reformatting by clang-format (the way you wro
Done


http://gerrit.cloudera.org:8080/#/c/4340/1/be/src/runtime/descriptors.h
File be/src/runtime/descriptors.h:

PS1, Line 239: threads
> ... after the descriptor table is opened ...
Done


Line 260:   std::vector<ExprContext*> partition_key_value_ctxs_;
> This was already like this but it feels a little wonky to have non-immutabl
Agree. It feels to me like this belongs in the new query-wide state, indexed by partition id or something like that. Added a TODO.


PS1, Line 436: Open
> Maybe PrepareAndOpenExprs()? I think we want to limit the scope of what we 
Agree. Done.


PS1, Line 439: Close
> CloseExprs()?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id179df645e500530f4418988f6ce64a03d669892
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4098: Open()/Close() partition exprs once per fragment instance.

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

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

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

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

Change subject: IMPALA-4098: Open()/Close() partition exprs once per fragment instance.
......................................................................

IMPALA-4098: Open()/Close() partition exprs once per fragment instance.

Partition exprs stored in the descriptor table can be referenced by multiple
exec nodes (and/or a data sink) within the same fragment, so the lifecycle
of those exprs (Prepare/Open/Close) is tied to the fragment and not to
a particular exec node.

A recent change exposed this improper lifecycle management because we cloned
the partition exprs before using them, but by that time the exprs had been
closed which caused the cloning function to hit a DCHECK.

The fix is to tie the lifecycle of those exprs to that of the fragment
instance.

Testing: I could reliably reproduce the bug by running this query in a loop:

set num_nodes=1;
select count(a.year), count(a.month), count(a.int_col),
       count(b.year), count(b.month), count(b.int_col)
from functional.alltypessmall a, functional.alltypessmall b;

After this patch I was not able to reproduce the bug anymore. I don't think
it makes sense to add a test specifically for this bug because our existing
tests already caught it, and the hit DCHECK does not exist anymore due to
restructuring.

Change-Id: Id179df645e500530f4418988f6ce64a03d669892
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-table-sink.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M be/src/runtime/plan-fragment-executor.cc
6 files changed, 59 insertions(+), 106 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id179df645e500530f4418988f6ce64a03d669892
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4098: Open()/Close() partition exprs once per fragment instance.

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

Change subject: IMPALA-4098: Open()/Close() partition exprs once per fragment instance.
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4340/2//COMMIT_MSG
Commit Message:

Line 10: exec nodes (and/or a data sink) within the same fragment, so the lifecycle
> do you mean fragment or fragment instance? please clarify. (and if you real
Fragment instance, of course. Fixed.


http://gerrit.cloudera.org:8080/#/c/4340/2/be/src/runtime/descriptors.h
File be/src/runtime/descriptors.h:

Line 438:   Status PrepareAndOpenExprs(RuntimeState* state) const;
> ..PartitionExprs()? (or do you anticipate throwing more exprs into this?)
Done. Should definitely not add more exprs in here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id179df645e500530f4418988f6ce64a03d669892
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4098: Open()/Close() partition exprs once per fragment instance.

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

Change subject: IMPALA-4098: Open()/Close() partition exprs once per fragment instance.
......................................................................


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id179df645e500530f4418988f6ce64a03d669892
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4098: Open()/Close() partition exprs once per fragment instance.

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

Change subject: IMPALA-4098: Open()/Close() partition exprs once per fragment instance.
......................................................................


Patch Set 1: Code-Review+1

(6 comments)

This is a lot simpler.

http://gerrit.cloudera.org:8080/#/c/4340/1/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:

Line 216:   for (const auto& entry: tdesc.hdfsTable.partitions) {
clang-format puts a space before :.


Line 512:     for (const auto& part_entry: hdfs_tbl->partition_descriptors()) {
space before : to avoid later reformatting by clang-format (the way you wrote it is my personal preference but no point arguing with the tool :))


http://gerrit.cloudera.org:8080/#/c/4340/1/be/src/runtime/descriptors.h
File be/src/runtime/descriptors.h:

PS1, Line 239: threads
... after the descriptor table is opened ...


Line 260:   std::vector<ExprContext*> partition_key_value_ctxs_;
This was already like this but it feels a little wonky to have non-immutable stateful things in the descriptor table. I think your change makes it a lot more explicit, which is good. I suppose there's no other natural place to store per-partition state.


PS1, Line 436: Open
Maybe PrepareAndOpenExprs()? I think we want to limit the scope of what we do in here, because mostly the descriptor table should just have static metadata. Also that way it won't look like you forgot to Prepare() this thing if you're just looking at the callsites.


PS1, Line 439: Close
CloseExprs()?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id179df645e500530f4418988f6ce64a03d669892
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4098: Open()/Close() partition exprs once per fragment instance.

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

Change subject: IMPALA-4098: Open()/Close() partition exprs once per fragment instance.
......................................................................


Patch Set 3: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id179df645e500530f4418988f6ce64a03d669892
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No