impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4883: Implement Codegen for Union
Date Thu, 23 Mar 2017 20:12:24 GMT
Michael Ho has posted comments on this change.

Change subject: IMPALA-4883: Implement Codegen for Union
......................................................................


Patch Set 1:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/6459/1/be/src/exec/union-node-ir.cc
File be/src/exec/union-node-ir.cc:

PS1, Line 28: TupleRow* dst_row = dst_batch->GetRow(dst_batch->AddRow());
Can this be not cross-compiled and we can make the caller pass dst_row directly instead ?


PS1, Line 32: dst_batch->tuple_data_pool()
Can the caller pass dst_batch->tuple_data_pool() to this function instead ? This avoid
compiling code which won't provide much performance boost.


Line 34:   dst_batch->CommitLastRow();
Also don't seem to be strictly needed to be cross-compiled. Please correct me if I misunderstood
anything.


http://gerrit.cloudera.org:8080/#/c/6459/1/be/src/exec/union-node.cc
File be/src/exec/union-node.cc:

PS1, Line 40: NULL
nullptr


PS1, Line 108: DCHECK(codegen_status.ok());
Don't think it's safe to always assume codegen succeeded. It can fail for legitimate reason
(e.g. we don't handle TYPE_CHAR). Please consider skipping this child expression if codegen
for it failed.


PS1, Line 114: NULL
nullptr


PS1, Line 231: IsNodeCodegenDisabled()
Wouldn't that be implied by codegen_union_materialize_expr_fns_.size() ==0 ?


PS1, Line 233: codegend_union_materialize_exprs_fns_.data()[child_idx_] == NULL
Isn't this the only check we care about ? Are the other two conditions needed ?


PS1, Line 237: codegend_union_materialize_exprs_fns_.data()[child_idx_]
codegend_union_materialize_exprs_fns_[child_idx]


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4107d27582ff5416172810364a6e76d3d93c439
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message