Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4314: Standardize on MT-related data structures ......................................................................
Patch Set 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/4853/1/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: Line 457: bool is_mt_execution = request.query_ctx.request.query_options.mt_dop > 0; > set in FE? didn't seem worth it. it would be a single bool instead of a single int with a '> 0' around it. http://gerrit.cloudera.org:8080/#/c/4853/2/be/src/scheduling/query-schedule.cc File be/src/scheduling/query-schedule.cc: Line 60: // extract TPlanFragments and order by fragment id > order by fragment idx? Done Line 70: DCHECK_EQ(fragment_exec_params_.size(), 0); > comment that we asserting this is the first call to Init() Done Line 244: if (fragment->partition.type == TPartitionType::UNPARTITIONED) { > Shouldn't this be a DCHECK instead? Every TStmtType::QUERY must have such a but this should be callable for a non-query stmt (so the coordinator doesn't need to understand at all times what it's dealing with). Line 264: const FragmentExecParams* fragment_params = &fragment_exec_params_[coord_fragment.idx]; > make this a FragmentExecParams& Done Line 265: DCHECK(fragment_params != nullptr); > weird DCHECK, fix by using const& as suggested above Done http://gerrit.cloudera.org:8080/#/c/4853/2/be/src/scheduling/query-schedule.h File be/src/scheduling/query-schedule.h: Line 211: // populated by Scheduler::Schedule (SimpleScheduler::ComputeFInstanceExecParams()) > now populated in Init() right? clarified http://gerrit.cloudera.org:8080/#/c/4853/2/be/src/scheduling/simple-scheduler.cc File be/src/scheduling/simple-scheduler.cc: Line 725: #if 0 > ? Done Line 750: // TODO: we'll need to revisit this strategy once we can partition joins > Maybe we should deal with this in the planner? I think the only way we can could think about that, but let's do that in a follow-on change. http://gerrit.cloudera.org:8080/#/c/4853/1/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: Line 351: // The node ids refer to scan nodes in fragments[].plan_tree > fragments[].plan (not plan_tree) Done http://gerrit.cloudera.org:8080/#/c/4853/1/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: Line 1000: boolean disableSpilling = > whitespace Done -- To view, visit http://gerrit.cloudera.org:8080/4853 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I465d0e15e2cf17cafe4c747d34c8f595d3645151 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
