Tim Armstrong has posted comments on this change. Change subject: Refactor RuntimeState and ExecEnv dependencies ......................................................................
Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/3108/3/be/src/runtime/runtime-filter-bank.h File be/src/runtime/runtime-filter-bank.h: Line 1: // Copyright 2016 Cloudera Inc. > Is this file changed from the file it was copied from (excluding declaratio Not substantially. I can change back to 2012 (not sure what the rule of thumb is here). http://gerrit.cloudera.org:8080/#/c/3108/3/be/src/runtime/runtime-filter.cc File be/src/runtime/runtime-filter.cc: Line 146 > Can you help me undersyand why this and line 149 are changing? It's to avoid storing the whole TQueryCtx object inline in the object (which implies that the whole thrift class header needs to be exposed to any file that includes runtime-state.h). It wasn't really necessary to do that, since only a couple of fields were used from it. Instead of copying the TQueryCtx from RuntimeState to here, we can just access it directly in the RuntimeState. Line 40: RuntimeFilterBank::RuntimeFilterBank(const TQueryCtx& query_ctx, RuntimeState* state) > Shouldn't these definitions be moved to runtime-filter-bank.cc? We could do something like that. The only awkward thing would be that runtime-filter.cc would be almost empty. http://gerrit.cloudera.org:8080/#/c/3108/3/be/src/runtime/runtime-filter.h File be/src/runtime/runtime-filter.h: Line 112 This field was deleted as part of the copy. http://gerrit.cloudera.org:8080/#/c/3108/3/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: Line 362: boost::scoped_ptr<RuntimeFilterBank> filter_bank_; > What is the likelihood of a performance regression from this extra indirect It looks like this is not referenced during actual query processing. In the scan and join node, the runtime filters are accessed via FilterContexts http://gerrit.cloudera.org:8080/#/c/3108/3/be/src/util/runtime-profile-counters.h File be/src/util/runtime-profile-counters.h: Line 1: // Copyright 2016 Cloudera Inc. > Is this file changed from the file it was copied from (excluding declaratio I can change back (I assume if there weren't substantial changes, you mean it should retain the old date). -- To view, visit http://gerrit.cloudera.org:8080/3108 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b246ad9c3681d649e7bfc969c7fa885c6242d84 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
