Skye Wanderman-Milne has posted comments on this change. Change subject: IMPALA-2548: Codegen Tuple::MaterializeExprs() and use in TopN node ......................................................................
Patch Set 6: (3 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, > I think a more reliable way would be to also compare the arg list and ret t I gave them the same signature so CodegenMaterializeExprs() always returns a function with the same signature (I thought it would be confusing otherwise), but I could change this. LLVM will complain if we try to call a function with the wrong arguments too. http://gerrit.cloudera.org:8080/#/c/1901/6/be/src/runtime/tuple.cc File be/src/runtime/tuple.cc: Line 327: GetPtrType(ExprContext::LLVM_CLASS_NAME)->getPointerTo(); > Sounds good to me if that doesn't make it too verbose to write certain expr '->getPointerTo' is fewer characters than 'codegen->GetPtrType' as well, so it's actually less verbose usually. 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 = > How about using vector::data() ? I'm fine with using vector.data(). I compiled a quick test program and it appears to generate the same instructions as &vector[0], The argument against it would be that it's inconsistent with the rest of the code base, but it also looks like the new (as of C++11) canonical access method, plus &vector[0] on an empty vector is technically undefined behavior (even though in practice it's the same as .data() apparently). I'll change it here and see if anyone else has an opinion. -- 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
