Dan Hecht has posted comments on this change. Change subject: IMPALA-4432: Handle internal codegen disabling properly ......................................................................
Patch Set 6: (4 comments) Thanks, this is much easier to follow now. It'd be good for Tim to take another pass now to make sure it still makes sense to him as well. http://gerrit.cloudera.org:8080/#/c/5105/6/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: PS6, Line 169: HasScalarFnToCodegen Maybe ScalarFnRequiresCodegen() or similar? i.e. to make it clear that not only do we codegen these, but that they require codegen. Line 171: /// Returns true if there is hint to disable codegen. This can be true for single node "a" Line 180: /// of the hint and all expressions in the fragment can be interpreted. given that this depends on the scalar_fns_to_codegen_ state which isn't query-static state, let's document at what point this routine can be used (e.g. after Prepare() phase or whatever). Line 194: /// which cannot be interpreted. I think this would be clearer without the double negative. eg.: 1. It's enabled by query option. 2. Enabled by the internal hint and all expressions can be interpreted. -- 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: 6 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
