Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9749 )

Change subject: IMPALA-6694: fix "buffer pool" child profile order
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9749/3/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

http://gerrit.cloudera.org:8080/#/c/9749/3/be/src/util/runtime-profile.cc@358
PS3, Line 358: insert_pos != children_.end()
> After looking at the profile in the Jira more I'm not sure anymore that I u
I think the thing that you're missing is that there's an "indent" flag that 
causes nested children to be printed without indentation. The flag is set on 
the first child ExecNode profile when building the ExecNode profile tree. E.g. 
if the plan is:

  2:HASH_JOIN --
  |            |
  V            V
  0:SCAN      1:SCAN

Then the RuntimeProfile tree has this shape:

        ----2:HASH_JOIN----
       /          |        \
   BufferPool   1:SCAN    0:SCAN

0 has "indent = false" and the remaining children have indent = true (see here 
https://github.com/apache/impala/blob/master/be/src/exec/exec-node.cc#L324 for 
how this happens)

It gets pretty-printed with the following structure, since the pretty-printing 
does depth-first search and only indents child profiles with indent = true.

  2:HASH_JOIN
    - A counter
    - Another counter
    BufferPool:
      - ...
    1:SCAN
      - ...
  0:SCAN
    - ...

The issue is if BufferPool becomes the last child, it still has indent=true and 
you get the following tree and pretty-printed output:

        ----2:HASH_JOIN----
       /          |        \
    1:SCAN    0:SCAN     BufferPool


  2:HASH_JOIN
    - A counter
    - Another counter
    1:SCAN
      - ...
  0:SCAN
    - ...
    BufferPool:
      - ...

So the tree structure is preserved but it gets pretty-printed wrong. As far as 
I'm aware only Impala's pretty-printing respects the indent flag - other tools 
I've seen that operate on the thrift profiles ignore it, so the ordering of 
profiles is messed up but the visual layout isn't quite as weird.



--
To view, visit http://gerrit.cloudera.org:8080/9749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I230f0673edf20a846fdb13191b7a292d329c1bb8
Gerrit-Change-Number: 9749
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <[email protected]>
Gerrit-Reviewer: Lars Volker <[email protected]>
Gerrit-Reviewer: Michael Ho <[email protected]>
Gerrit-Reviewer: Tim Armstrong <[email protected]>
Gerrit-Comment-Date: Wed, 28 Mar 2018 16:02:15 +0000
Gerrit-HasComments: Yes

Reply via email to