Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11992 )
Change subject: IMPALA-2343: Add lifecycle timeline to plan nodes ...................................................................... Patch Set 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/11992/7/be/src/exec/exec-node-util.h File be/src/exec/exec-node-util.h: http://gerrit.cloudera.org:8080/#/c/11992/7/be/src/exec/exec-node-util.h@57 PS7, Line 57: Fetched > nit: maybe reword to "First Batch Requested" to indicate that it is still i Done http://gerrit.cloudera.org:8080/#/c/11992/7/be/src/exec/exec-node.h File be/src/exec/exec-node.h: http://gerrit.cloudera.org:8080/#/c/11992/7/be/src/exec/exec-node.h@96 PS7, Line 96: virtual Status Open(RuntimeState* state) WARN_UNUSED_RESULT; Responding to your top-level comment here so we can have a proper thread. I'd thought about that but decided not to bite it off right now. I also thought about changing this so that there's a non-virtual GetNext()/Open() function in the base class that calls out to an implementation but I don't think that actually works since there are some special case ExecNodes that don't have the standard preamble. I feel like the macro approach might be easier to understand overall even if it leaves in some boilerplate. http://gerrit.cloudera.org:8080/#/c/11992/7/be/src/exec/exec-node.h@260 PS7, Line 260: were > nit: was Done http://gerrit.cloudera.org:8080/#/c/11992/7/be/src/exec/hbase-scan-node.cc File be/src/exec/hbase-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/11992/7/be/src/exec/hbase-scan-node.cc@153 PS7, Line 153: // For GetNext, most of the time is spent in HBaseTableScanner::ResultScanner_next, : // but there's still some considerable time inside here. > I don't think this comment helps much and would be good with dropping it al Done http://gerrit.cloudera.org:8080/#/c/11992/7/be/src/exec/kudu-scan-node.cc File be/src/exec/kudu-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/11992/7/be/src/exec/kudu-scan-node.cc@92 PS7, Line 92: DCHECK(row_batch != NULL); Removed this DCHECK since it seems unnecessary (unlikely to be triggered and we'd find out that it was null soon enough anyway if it was). http://gerrit.cloudera.org:8080/#/c/11992/7/be/src/exec/kudu-scan-node.cc@96 PS7, Line 96: SCOPED_TIMER(materialize_tuple_timer()); > Can you add a brief comment why this timer gets initialized after the pream I was kind of ignoring this because I didn't want to deal with it :). I think this timer is actually in the wrong place entirely. The equivalent timer in HDFS is measured in the scanner thread, since that's the thread that actually does the work to materialise the batch. I can fix in this patch if you are ok with that. http://gerrit.cloudera.org:8080/#/c/11992/7/be/src/exec/singular-row-src-node.cc File be/src/exec/singular-row-src-node.cc: http://gerrit.cloudera.org:8080/#/c/11992/7/be/src/exec/singular-row-src-node.cc@33 PS7, Line 33: ScopedOpenEventAdder ea(this); > From the FE comment in SingularRowSrcNode is seems that this will always ru Good point. I'll just revert all the changes to this file. http://gerrit.cloudera.org:8080/#/c/11992/7/be/src/exec/unnest-node.cc File be/src/exec/unnest-node.cc: http://gerrit.cloudera.org:8080/#/c/11992/7/be/src/exec/unnest-node.cc@142 PS7, Line 142: // Avoid expensive query maintenance overhead for small collections. > Did you omit the instrumentation here because it is expensive? Yep, and this is always in a subplan too. I removed the instrumentation about too, it wasn't necessary. -- To view, visit http://gerrit.cloudera.org:8080/11992 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I15341bdb15022bad9814882689ce5cb2939f4653 Gerrit-Change-Number: 11992 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 28 Nov 2018 21:05:06 +0000 Gerrit-HasComments: Yes
