Michael Ho has posted comments on this change. Change subject: IMPALA-2548: Codegen Tuple::MaterializeExprs() and use in TopN node ......................................................................
Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/1901/6/be/src/exec/topn-node.cc File be/src/exec/topn-node.cc: Line 84: insert_batch_fn = codegen->ReplaceCallSites(insert_batch_fn, false, > No way to guarantee, it's just synchronized with the cross-compiled code. I think a more reliable way would be to also compare the arg list and ret type too but not sure how hard it is with our current infrastructure. http://gerrit.cloudera.org:8080/#/c/1901/6/be/src/runtime/tuple.cc File be/src/runtime/tuple.cc: Line 249: trunc i64 %src to i1 > We use trunc to truncate the val field of the IntVal %src, which is returne I see. It's checking the is_null field of AnyVal. I mistook it for checking ptr == NULL. Line 327: GetPtrType(ExprContext::LLVM_CLASS_NAME)->getPointerTo(); > No good reason, I think no one knew about getPointerTo() when GetPtrType() Sounds good to me if that doesn't make it too verbose to write certain expressions. Line 348: // Value* pool_arg = args[4]; // unused, we hardcode 'pool's ptr instead : // Value* non_null_string_values_arg = args[5]; // unused : // Value* total_string_arg = args[6]; // unused > I don't think they'd fit as-is? I like having these here because it helps n That's fine. I was thinking that a one line comment like 'The rest of args aren't used so not specifying them here.' http://gerrit.cloudera.org:8080/#/c/1901/6/be/src/runtime/tuple.h File be/src/runtime/tuple.h: Line 122: std::vector<StringValue*>* non_null_string_values = > You mean passing in &materialize_expr_ctxs[0] below? AFAIK that's the idiom How about using vector::data() ? -- To view, visit http://gerrit.cloudera.org:8080/1901 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib422a8d50303c21c6a228675157bf867e8619444 Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-HasComments: Yes
