Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13289 )
Change subject: expr-test: use gtest parameterization ...................................................................... Patch Set 1: (1 comment) This seems like an improvement. I think the previous approach was just from lack of knowledge about gtest. I know I did something similar with buffer-pool-test too :) 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 Actually this can just be unconditional because it has no effect if codegen is disabled. I looked at the original code and it was an artefact of how that code was structured instead of intentional. -- 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 <t...@apache.org> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 09 May 2019 00:55:56 +0000 Gerrit-HasComments: Yes
