Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/24353 )
Change subject: IMPALA-12956: Add profile summary output ...................................................................... Patch Set 9: (6 comments) Thanks for your patience in the review! Just have few comments. http://gerrit.cloudera.org:8080/#/c/24353/7/be/src/util/impala-profile-tool.cc File be/src/util/impala-profile-tool.cc: http://gerrit.cloudera.org:8080/#/c/24353/7/be/src/util/impala-profile-tool.cc@267 PS7, Line 267: return found; > Fixed in PS9. Bytes Spilled now sums ScratchBytesWritten from direct host c Oops, the new implementation seems more complex than the original one.. Now I'm OK to both. http://gerrit.cloudera.org:8080/#/c/24353/7/be/src/util/impala-profile-tool.cc@354 PS7, Line 354: ce& sequence : summary_ Any reason that we rollback to not using ParseUtil::ParseMemSpec()? http://gerrit.cloudera.org:8080/#/c/24353/9/be/src/util/progress-util.cc File be/src/util/progress-util.cc: http://gerrit.cloudera.org:8080/#/c/24353/9/be/src/util/progress-util.cc@31 PS9, Line 31: ss << "0%)"; setw only affects the next item. On L33 it's the percent value without "%)". I think this should be ss << "0" << "%)"; http://gerrit.cloudera.org:8080/#/c/24353/9/be/src/util/summary-util.h File be/src/util/summary-util.h: http://gerrit.cloudera.org:8080/#/c/24353/9/be/src/util/summary-util.h@21 PS9, Line 21: #include <string> nit: why do we need this now? http://gerrit.cloudera.org:8080/#/c/24353/9/be/src/util/summary-util.cc File be/src/util/summary-util.cc: http://gerrit.cloudera.org:8080/#/c/24353/9/be/src/util/summary-util.cc@20 PS9, Line 20: #include <sstream> nit: why do we need this now? http://gerrit.cloudera.org:8080/#/c/24353/9/be/src/util/summary-util.cc@22 PS9, Line 22: nit: please don't add blank lines like this -- To view, visit http://gerrit.cloudera.org:8080/24353 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I35da958106ffb35b14ef867da50ed23ffda0a45b Gerrit-Change-Number: 24353 Gerrit-PatchSet: 9 Gerrit-Owner: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 07 Jul 2026 08:28:23 +0000 Gerrit-HasComments: Yes
