Dan Hecht 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. what is this trying to say? that the hint is always respected by this exec node? 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. it's not clear that we mean it was disabled because of a global internal decision, as opposed to, say, being disabled internally to this node). 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()? 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" 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 just adds confusion IMO and should be deleted. Line 115: // than 20 fixed arguments cannot be interpreted. not clear from the comment whether conditions 1, 2 & 3 are "anded" or "or'ed" together in the decision process. maybe "without codegen if any of these are true" PS4, Line 131: 20 nit: ...a predetermined number of... otherwise, this comment will likely get out of date next time we update that macro definition. 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 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's taking the hint into account, because the hint doesn't really dictate whether codegen is disabled or not. I think we need a better name for this, something like CodegenQOptionOrHintEnabled(), except that's kinda long. 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 function if CodegenDisabledInternall() is only advisory. PS4, Line 286: This behavior should : /// change how should it change? or if you're saying the JIRA documents how it should be changed, how about just saying: TODO: fix IMPALA-4233? 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_hint? The fact that it's internal doesn't really tell you that it's advisory, but "hint" does. -- 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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
