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 8: (8 comments) Thanks for the comments. All good points. 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 Done 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? moved to .cc 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 whe Done 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( Yeah that's true. I missed updating this comment - an earlier iteration of the patch tried to have some rules about when it was valid to call a subexpr. 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 Done 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 Done 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 Done 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 bui Yeah this is true, test_partitioned_insert is actually broken. I did run this test directly and it passes. Filing IMPALA-8410 since I think there's probably some thought required to do it the right way, given the concern about runtime above. I'm open to fixing in this patch, since I'm adding the test, but it seems easier to separate it. -- 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 21:41:49 +0000 Gerrit-HasComments: Yes
