Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs ...................................................................... Patch Set 12: (10 comments) http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exec/grouping-aggregator.cc File be/src/exec/grouping-aggregator.cc: http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exec/grouping-aggregator.cc@119 PS12, Line 119: intermediate_row_desc_, /* is_entry_point */ false, state)); > nit: indentation Done - ran clang-format on it. http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h File be/src/exprs/scalar-expr.h: http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h@110 PS12, Line 110: interpreted code. > (e.g. an operator which doesn't support codegen yet). Done http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h@117 PS12, Line 117: /// TODO: Fix subclasses which call GetCodegendComputeFnWrapper() to not call interpreted : /// functions. > Stale ? Still relevant, but could be clarified. Updated comment. http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h@170 PS12, Line 170: /// compilation. Returns error status on failure. > Can you please also add something similar to below: Done http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h@246 PS12, Line 246: for each > to be overridden by Done http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h@388 PS12, Line 388: If this is true, then after the expressions are codegen'd, : /// then 'codegend_compute_fn_' is non-NULL. > If this is true, 'codegend_compute_fn_' will be set to the JIT'd function p Done http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.cc File be/src/exprs/scalar-expr.cc: http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.cc@86 PS12, Line 86: /// > nit: // Done http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.inline.h File be/src/exprs/scalar-expr.inline.h: http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.inline.h@24 PS12, Line 24: // > nit: /// Done http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.inline.h@28 PS12, Line 28: ScalarFnCall > ScalarExpr Done http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-fn-call.cc File be/src/exprs/scalar-fn-call.cc: http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-fn-call.cc@95 PS12, Line 95: // CHAR or VARCHAR are not supported as input arguments or return values for UDFs. > stale ? Done -- To view, visit http://gerrit.cloudera.org:8080/12797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11 Gerrit-Change-Number: 12797 Gerrit-PatchSet: 12 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Sat, 11 May 2019 00:42:41 +0000 Gerrit-HasComments: Yes
