Marcel Kornacker has posted comments on this change. Change subject: IMPALA-2548: Codegen Tuple::MaterializeExprs() and use in TopN node ......................................................................
Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/1901/10/be/src/runtime/descriptors.h File be/src/runtime/descriptors.h: Line 395: static const char* LLVM_CLASS_NAME; does everybody need access to this? if not, make private and add friends http://gerrit.cloudera.org:8080/#/c/1901/10/be/src/runtime/tuple.cc File be/src/runtime/tuple.cc: Line 208: SlotDescriptor* slot_desc = desc.slots()[i]; instead of generating ir by hand below, could you do this by a) writing a function for just the body of the loop b) writing a codegen helper function that given a function and looping params, creates an unrolled loop this isn't as good as doing the unrolling automatically in cross-compiled functions, but it would be a step in the right direction, and it'll be easier to write and review than the function below. http://gerrit.cloudera.org:8080/#/c/1901/10/be/src/runtime/tuple.h File be/src/runtime/tuple.h: Line 123: template <bool collect_string_vals, bool no_pool> what is the point of templatizing this? if there's a codegen'd version of it, the dead branches should disappear anyway, no? Line 143: static const char* MATERIALIZE_EXPRS_SYMBOL; move to private (and make whoever needs this a friend, hopefully that'll be very few classes) -- 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: 10 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Skye Wanderman-Milne <s...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Skye Wanderman-Milne <s...@cloudera.com> Gerrit-HasComments: Yes
