Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080: Codegen once per fragment ...................................................................... Patch Set 8: (5 comments) Looking good! I have only minor comments. It looks like there are quite a few TODOs for comments, so I'm assuming you're going to get back to those. http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/fragment-state.h File be/src/runtime/fragment-state.h: http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/fragment-state.h@42 PS8, Line 42: // TODO: add desc Missed this TODO http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/fragment-state.h@168 PS8, Line 168: /// TODO: add description Same here http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/fragment-state.cc File be/src/runtime/fragment-state.cc: http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/fragment-state.cc@84 PS8, Line 84: query_state_->ErrorDuringFragmentCodegen(codegen_status_); It would be good to do this outside of the critical section so that we don't need to worry about lock ordering between codegen_lock_ and QueryState::status_lock_ http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/query-state.cc@548 PS8, Line 548: { nit: extra braces for lock scope not needed http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/udf/udf-internal.h File be/src/udf/udf-internal.h: http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/udf/udf-internal.h@174 PS7, Line 174: static int GetConstFnAttr(bool uses_decimal_v2, > I decided to just pass this bool here (just renamed it to uses_decimal_v2) ok sounds fine -- To view, visit http://gerrit.cloudera.org:8080/15408 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452 Gerrit-Change-Number: 15408 Gerrit-PatchSet: 8 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 27 Mar 2020 17:45:53 +0000 Gerrit-HasComments: Yes
