Lars Volker 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: (6 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 in progress? 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@260 PS7, Line 260: were nit: was 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 altogether. 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@96 PS7, Line 96: SCOPED_TIMER(materialize_tuple_timer()); Can you add a brief comment why this timer gets initialized after the preamble? 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 run >in a subplan: A SingularRowSrcNode returns the current row that is being processed by its containing SubplanNode. A SingularRowSrcNode can only appear in the plan tree of a SubplanNode. A SingularRowSrcNode returns its parent's smap such that substitutions are appropriately applied within the SubplanNode's second child. In that case, do we need to instrument this method at all? 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? -- 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-Comment-Date: Wed, 28 Nov 2018 01:39:59 +0000 Gerrit-HasComments: Yes
