Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15663 )
Change subject: IMPALA-9422: Re-visit and improve join node and builder's counters ...................................................................... Patch Set 1: (4 comments) The concept makes sense to me. Had some questions, and also a concern about the example profile. http://gerrit.cloudera.org:8080/#/c/15663/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15663/1//COMMIT_MSG@24 PS1, Line 24: non-child: 0.000ns, This non-child time doesn't seem right, we should be counting some time against the join node itself - at least PRobeTime. This non-child time in the join is kinda weird, cause we explicitly calculate local time, rather than just subtracting the child time. http://gerrit.cloudera.org:8080/#/c/15663/1/be/src/exec/partitioned-hash-join-builder.h File be/src/exec/partitioned-hash-join-builder.h: http://gerrit.cloudera.org:8080/#/c/15663/1/be/src/exec/partitioned-hash-join-builder.h@586 PS1, Line 586: Status SendHelper(RowBatch* build_batch); I think a better name is possible based on what it actually does, e.g. AddBatch() http://gerrit.cloudera.org:8080/#/c/15663/1/be/src/exec/partitioned-hash-join-builder.h@590 PS1, Line 590: FlushFinalHelper Maybe FinalizeBuild() http://gerrit.cloudera.org:8080/#/c/15663/1/be/src/exec/partitioned-hash-join-builder.cc File be/src/exec/partitioned-hash-join-builder.cc: http://gerrit.cloudera.org:8080/#/c/15663/1/be/src/exec/partitioned-hash-join-builder.cc@743 PS1, Line 743: SCOPED_TIMER(profile()->total_time_counter()); Maybe we should be actually starting and stopping the inactive timer and this timer at exactly the same time, e.g. with the SCOPED_TIMER2 macro. Since I think we want all the time to be attributed to one or the other. -- To view, visit http://gerrit.cloudera.org:8080/15663 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I604075a2c8efcff26705fb39672f29f309b2ed97 Gerrit-Change-Number: 15663 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 07 Apr 2020 16:09:02 +0000 Gerrit-HasComments: Yes
