Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13289 )
Change subject: expr-test: use gtest parameterization ...................................................................... Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/13289/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/13289/1/be/src/exprs/expr-test.cc@282 PS1, Line 282: // TODO(todd): why are these settings only for this case? I think this only needs to be for (!disable_codegen_). These are important in that case so codegen doesn't get auto-disabled. We could just set them unconditionally though even so that the planner doesn't do anything clever in any cases. http://gerrit.cloudera.org:8080/#/c/13289/1/be/src/exprs/expr-test.cc@9296 PS1, Line 9296: // TODO(todd) why don't we test the false/true case? IIRC just because it didn't add much coverage and adds a lot to test runtime (running this test with codegen enabled is really slow). It was a bit of a fight to even convince people that we should run it in one configuration with codegen enabled. Mostly we get the best backend codegened/interpreted coverage from running the unmodified (i.e. more complex) expr trees enabled. Running the trees in interpreted mode should be enough to validate correctness of the rewrites. So enabling the remaining combination might provide some additional incidental coverage from codegening some additional expr tree shapes but mostly it isn't that interesting since the majorit y of expressions get folded to a constant anyway. -- To view, visit http://gerrit.cloudera.org:8080/13289 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idc9fb24ad62b4aa2e120a99d74ae04bb221c034b Gerrit-Change-Number: 13289 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 09 May 2019 00:53:53 +0000 Gerrit-HasComments: Yes
