Michael Ho has posted comments on this change. Change subject: IMPALA-4432: Handle internal codegen disabling properly ......................................................................
Patch Set 4: (12 comments) http://gerrit.cloudera.org:8080/#/c/5105/4/be/src/exec/exchange-node.cc File be/src/exec/exchange-node.cc: Line 94: DCHECK(!state->CodegenDisabled()); > why are these DCHECKs correct if the "internal" flag is only advisory? i.e. Yes, the hint is always respected by the exec node. The intent is to codegen the ScalarFnCall only if the hint is present. However, this seems unnecessarily complicated to optimize for these corner cases (e.g. IR UDF or UDF with more than 20 fixed arguments). Updated the patch to codegen everything if the hint is ignored due to uninterpretable UDF. http://gerrit.cloudera.org:8080/#/c/5105/4/be/src/exec/exec-node.cc File be/src/exec/exec-node.cc: PS4, Line 464: internally > "automatically"? "disabled internally" seems confusing to an end user (i.e Sure. Rephrased to "disabled due to optimization hints". http://gerrit.cloudera.org:8080/#/c/5105/4/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: Line 297: } > why isn't this AddCodegenDisabledMessage()? Done http://gerrit.cloudera.org:8080/#/c/5105/4/be/src/exprs/scalar-fn-call.cc File be/src/exprs/scalar-fn-call.cc: PS4, Line 113: internally > again, I think this would be clearer if we called it a "hint" Done PS4, Line 113: Internal codegen : // disabling is ignored if the UDF cannot be interpreted. > this sentence seems to just restate the first sentence, right? if so, it ju Done Line 115: // than 20 fixed arguments cannot be interpreted. > not clear from the comment whether conditions 1, 2 & 3 are "anded" or "or'e Done PS4, Line 131: 20 > nit: ...a predetermined number of... Done http://gerrit.cloudera.org:8080/#/c/5105/4/be/src/exprs/scalar-fn-call.h File be/src/exprs/scalar-fn-call.h: PS4, Line 127: beeb > been Done http://gerrit.cloudera.org:8080/#/c/5105/4/be/src/runtime/plan-fragment-executor.cc File be/src/runtime/plan-fragment-executor.cc: PS4, Line 249: CodegenDisabled > As mentioned before, I find it confusing that this "hides" the fact that it I refactor this entire clause into a function called ShouldCodegen() so there is a unified location in which the policy is encoded. Also changed the policy to codegen the exec tree too if the hint is ignored to keep things simpler. http://gerrit.cloudera.org:8080/#/c/5105/4/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: Line 175: return CodegenDisabledByQueryOption() || CodegenDisabledInternally(); > as mentioned elsewhere, it's hard to see how this is a good name for this f Refactored to ShouldCodegen(). Also renamed CodegenDisabledInternally() to CodegenDisabledByHint(). PS4, Line 286: This behavior should : /// change > how should it change? or if you're saying the JIRA documents how it should Updated the comment to add more details on the expected behavior after IMPALA-4233 is fixed. http://gerrit.cloudera.org:8080/#/c/5105/4/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: PS4, Line 318: internal > if it's only advisory, how about naming it something like disable_codegen_h Done -- 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: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
