Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8196 )
Change subject: IMPALA-4236: Codegen CopyRows() for select nodes ...................................................................... Patch Set 9: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/8196/8/be/src/exec/select-node.cc File be/src/exec/select-node.cc: http://gerrit.cloudera.org:8080/#/c/8196/8/be/src/exec/select-node.cc@72 PS8, Line 72: Status( > I believe finalize function does not fail frequently, so shouldn't affect p It's probably quite clear what the call path is since this is the codegen function for an exec node (as opposed to an Expr). It's true that failure should rarely occur on this path so may be it's not that bad after all. If anything, I would print the IR function which fails to finalize if we aren't doing it already. http://gerrit.cloudera.org:8080/#/c/8196/9/tests/query_test/test_codegen.py File tests/query_test/test_codegen.py: http://gerrit.cloudera.org:8080/#/c/8196/9/tests/query_test/test_codegen.py@52 PS9, Line 52: assert_codegen_enabled(result.runtime_profile, [1]) Optionally, you can consider adding a test in which codegen fails. Not strictly needed for this patch. -- To view, visit http://gerrit.cloudera.org:8080/8196 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie0d496d004418468e16b6f564f90f45ebbf87c1e Gerrit-Change-Number: 8196 Gerrit-PatchSet: 9 Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 23 Oct 2017 23:26:39 +0000 Gerrit-HasComments: Yes
