Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs ...................................................................... Patch Set 8: (8 comments) http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/codegen/codegen-anyval.cc File be/src/codegen/codegen-anyval.cc: http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/codegen/codegen-anyval.cc@37 PS8, Line 37: nit: whitespace http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/codegen/llvm-codegen.h@44 PS8, Line 44: #include "runtime/collection-value.h" Is this needed? http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.h File be/src/exprs/scalar-expr.h: http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.h@292 PS8, Line 292: /// TODO: not all root exprs need to be entry point, e.g. if only called from a This TODO might make more sense in ScalarExpr::Create in scalar-expr.cc where the actual call with is_entry_point=true is. http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.cc File be/src/exprs/scalar-expr.cc: http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.cc@402 PS8, Line 402: // Ensure that Get*Val() can be called on each child by calling with : // is_codegen_entry_point=true. I'm a little confused by this comment - if I understand correctly, Get*Val() can always be called on any ScalarExpr since it can just fall back to the interpreted path, so the point of this is to make sure that when Get*Val() is called on a child it goes through the codegen'd path if possible? http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.inline.h File be/src/exprs/scalar-expr.inline.h: http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.inline.h@24 PS8, Line 24: typedef BooleanVal (*BooleanValWrapper)(ScalarExprEvaluator*, const TupleRow*); These could be moved into the macro below http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-fn-call.cc File be/src/exprs/scalar-fn-call.cc: http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-fn-call.cc@485 PS8, Line 485: \ nit: whitespace http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/udf/udf-internal.h File be/src/udf/udf-internal.h: http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/udf/udf-internal.h@300 PS8, Line 300: Also is denser I find this comment confusing. Denser than what? (I assume you mean denser than the previous layout, but someone reading this for the first time wouldn't necessarily have that context) http://gerrit.cloudera.org:8080/#/c/12797/8/tests/query_test/test_tpcds_queries.py File tests/query_test/test_tpcds_queries.py: http://gerrit.cloudera.org:8080/#/c/12797/8/tests/query_test/test_tpcds_queries.py@528 PS8, Line 528: def test_expr_insert(self, vector): I think this test as is will not actually get run in any of our regular builds (something that I stumbled upon when working on https://gerrit.cloudera.org/#/c/12960/). I'm assuming that's not your intention. The issue is that even in EXHAUSTIVE builds, we only run test cases that have a workload of 'functional-query' in exhaustive mode (see how we set the values of --exploration_strategy and --workload_exploration_strategy in bin/run-all-tests.sh), so the check in add_test_dimensions() that disables this in core mode effectively disables it entirely. -- 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: 8 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: Fri, 12 Apr 2019 19:59:59 +0000 Gerrit-HasComments: Yes
