impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4314: Standardize on MT-related data structures
Date Thu, 27 Oct 2016 16:23:19 GMT
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 <marcel@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message