You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Alex Behm (Code Review)" <ge...@cloudera.org> on 2016/10/23 19:46:59 UTC
[Impala-ASF-CR] IMPALA-3586: Clean up union-node.h/cc to enable improvements.
Alex Behm has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4817
Change subject: IMPALA-3586: Clean up union-node.h/cc to enable improvements.
......................................................................
IMPALA-3586: Clean up union-node.h/cc to enable improvements.
This patch does not address IMPALA-3586, but it cleans up the
code in union-node.h/cc to make it easier to implement those
perf improvements.
The major simplification is to remove conjunct evaluation since
the planner does not assigns conjuncts to a union-node anymore.
Conjuncts are always pushed to the union operands.
Change-Id: Ia5fc23985e8d51acb8a6920717ce4e2f0254fe70
---
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
2 files changed, 90 insertions(+), 128 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/4817/1
--
To view, visit http://gerrit.cloudera.org:8080/4817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia5fc23985e8d51acb8a6920717ce4e2f0254fe70
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-3586: Clean up union-node.h/cc to enable improvements.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-3586: Clean up union-node.h/cc to enable improvements.
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5fc23985e8d51acb8a6920717ce4e2f0254fe70
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: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3586: Clean up union-node.h/cc to enable improvements.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3586: Clean up union-node.h/cc to enable improvements.
......................................................................
Patch Set 1: Code-Review+1
Carry Henry's +1
--
To view, visit http://gerrit.cloudera.org:8080/4817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5fc23985e8d51acb8a6920717ce4e2f0254fe70
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: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3586: Clean up union-node.h/cc to enable improvements.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3586: Clean up union-node.h/cc to enable improvements.
......................................................................
Patch Set 3: Code-Review+2
rebase
--
To view, visit http://gerrit.cloudera.org:8080/4817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5fc23985e8d51acb8a6920717ce4e2f0254fe70
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: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3586: Clean up union-node.h/cc to enable improvements.
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-3586: Clean up union-node.h/cc to enable improvements.
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/4817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5fc23985e8d51acb8a6920717ce4e2f0254fe70
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: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3586: Clean up union-node.h/cc to enable improvements.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3586: Clean up union-node.h/cc to enable improvements.
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4817/1/be/src/exec/union-node.cc
File be/src/exec/union-node.cc:
PS1, Line 72: for (int i = 0; i < const_expr_lists_.size(); ++i) {
: RETURN_IF_ERROR(Expr::Prepare(
: const_expr_lists_[i], state, row_desc(), expr_mem_tracker()));
: AddExprCtxsToFree(const_expr_lists_[i]);
: DCHECK_EQ(const_expr_lists_[i].size(), tuple_desc_->slots().size());
: }
:
: // Prepare result expr lists.
: for (int i = 0; i < child_expr_lists_.size(); ++i) {
: RETURN_IF_ERROR(Expr::Prepare(
: child_expr_lists_[i], state, child(i)->row_desc(), expr_mem_tracker()));
: AddExprCtxsToFree(child_expr_lists_[i]);
: DCHECK_EQ(child_expr_lists_[i].size(), tuple_desc_->slots().size());
: }
> ranged-for loops as well?
Done for the first loop.
The second loop needs child(i)->row_desc().
PS1, Line 220: for (int i = 0; i < const_expr_lists_.size(); ++i) {
: Expr::Close(const_expr_lists_[i], state);
: }
: for (int i = 0; i < child_expr_lists_.size(); ++i) {
: Expr::Close(child_expr_lists_[i], state);
: }
> While you're here, can you rewrite as two ranged-for loops?
Done
http://gerrit.cloudera.org:8080/#/c/4817/1/be/src/exec/union-node.h
File be/src/exec/union-node.h:
Line 58: /// Const exprs materialized by this node. These exprs don't refer to any children.
> Add that these are only materialized by the first fragment instance to avoi
Done
--
To view, visit http://gerrit.cloudera.org:8080/4817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5fc23985e8d51acb8a6920717ce4e2f0254fe70
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: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3586: Clean up union-node.h/cc to enable improvements.
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3586: Clean up union-node.h/cc to enable improvements.
......................................................................
Patch Set 1: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4817/1/be/src/exec/union-node.cc
File be/src/exec/union-node.cc:
PS1, Line 72: for (int i = 0; i < const_expr_lists_.size(); ++i) {
: RETURN_IF_ERROR(Expr::Prepare(
: const_expr_lists_[i], state, row_desc(), expr_mem_tracker()));
: AddExprCtxsToFree(const_expr_lists_[i]);
: DCHECK_EQ(const_expr_lists_[i].size(), tuple_desc_->slots().size());
: }
:
: // Prepare result expr lists.
: for (int i = 0; i < child_expr_lists_.size(); ++i) {
: RETURN_IF_ERROR(Expr::Prepare(
: child_expr_lists_[i], state, child(i)->row_desc(), expr_mem_tracker()));
: AddExprCtxsToFree(child_expr_lists_[i]);
: DCHECK_EQ(child_expr_lists_[i].size(), tuple_desc_->slots().size());
: }
ranged-for loops as well?
PS1, Line 220: for (int i = 0; i < const_expr_lists_.size(); ++i) {
: Expr::Close(const_expr_lists_[i], state);
: }
: for (int i = 0; i < child_expr_lists_.size(); ++i) {
: Expr::Close(child_expr_lists_[i], state);
: }
While you're here, can you rewrite as two ranged-for loops?
http://gerrit.cloudera.org:8080/#/c/4817/1/be/src/exec/union-node.h
File be/src/exec/union-node.h:
Line 58: /// Const exprs materialized by this node. These exprs don't refer to any children.
Add that these are only materialized by the first fragment instance to avoid duplication.
--
To view, visit http://gerrit.cloudera.org:8080/4817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5fc23985e8d51acb8a6920717ce4e2f0254fe70
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3586: Clean up union-node.h/cc to enable improvements.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Hello Henry Robinson,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4817
to look at the new patch set (#2).
Change subject: IMPALA-3586: Clean up union-node.h/cc to enable improvements.
......................................................................
IMPALA-3586: Clean up union-node.h/cc to enable improvements.
This patch does not address IMPALA-3586, but it cleans up the
code in union-node.h/cc to make it easier to implement those
perf improvements.
The major simplification is to remove conjunct evaluation since
the planner does not assigns conjuncts to a union-node anymore.
Conjuncts are always pushed to the union operands.
Change-Id: Ia5fc23985e8d51acb8a6920717ce4e2f0254fe70
---
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
2 files changed, 95 insertions(+), 133 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/4817/2
--
To view, visit http://gerrit.cloudera.org:8080/4817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia5fc23985e8d51acb8a6920717ce4e2f0254fe70
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: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-3586: Clean up union-node.h/cc to enable improvements.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3586: Clean up union-node.h/cc to enable improvements.
......................................................................
Patch Set 2:
This patch requires the fix for IMPALA-4336.
See: https://gerrit.cloudera.org/#/c/4815/
I'll hold off until that one gets reviewed/in.
--
To view, visit http://gerrit.cloudera.org:8080/4817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5fc23985e8d51acb8a6920717ce4e2f0254fe70
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: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3586: Clean up union-node.h/cc to enable improvements.
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.
Change subject: IMPALA-3586: Clean up union-node.h/cc to enable improvements.
......................................................................
IMPALA-3586: Clean up union-node.h/cc to enable improvements.
This patch does not address IMPALA-3586, but it cleans up the
code in union-node.h/cc to make it easier to implement those
perf improvements.
The major simplification is to remove conjunct evaluation since
the planner does not assigns conjuncts to a union-node anymore.
Conjuncts are always pushed to the union operands.
Change-Id: Ia5fc23985e8d51acb8a6920717ce4e2f0254fe70
Reviewed-on: http://gerrit.cloudera.org:8080/4817
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
2 files changed, 95 insertions(+), 133 deletions(-)
Approvals:
Internal Jenkins: Verified
Alex Behm: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/4817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia5fc23985e8d51acb8a6920717ce4e2f0254fe70
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: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins