Michael Ho has posted comments on this change. Change subject: IMPALA-4432: Handle internal codegen disabling properly ......................................................................
Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/5105/5/be/src/exec/exec-node.cc File be/src/exec/exec-node.cc: PS5, Line 463: CodegenDisabledByHint( > is this correct? we might still codegen if there's an IR udf, right? See al Good point. Fixed in the new patch. http://gerrit.cloudera.org:8080/#/c/5105/5/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: PS5, Line 171: codegen is disabled due to hints > is this true? doesn't this just indicate whether the hint would like to di Rephrased to "if there is hint to disable codegen." PS5, Line 175: CodegenDisabledByHint > if my above question is right, then this should be called CodegenDisableHin The new patch introduces both CodegenDisableHint() and CodegenDisabledByHint(). The former is needed in ScalarFnCall::Prepare(). Ideally, if there is hint to disable codegen, we should only codegen expressions which cannot be interpreted. If we use CodegenDisabledByHint() (with the suggested change) in ScalarFnCall::Prepare() and once an expression is added to scalar_fn_calls_to_codegen_, all subsequent ScalarFnCall expressions will be codegen'd. This makes it a bit non-deterministic which expressions will be codegen'd in the presence of the disabling hints as it depends on the order of when Prepare() is called for the non-interpretable expression. That said, the expressions may still be codegen'd when the operator is codegen'd but this logic may affect fe-support.cc and the above may matter in that case. -- To view, visit http://gerrit.cloudera.org:8080/5105 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0b6a9ed723c64ba21b861608583cc9b6607d3397 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
