Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/24353 )
Change subject: IMPALA-12956: Add profile summary output ...................................................................... Patch Set 6: (7 comments) Thanks for adding the new functionalities! I still need some time to complete the review. Post some comments first. http://gerrit.cloudera.org:8080/#/c/24353/6/be/src/util/impala-profile-tool.cc File be/src/util/impala-profile-tool.cc: http://gerrit.cloudera.org:8080/#/c/24353/6/be/src/util/impala-profile-tool.cc@84 PS6, Line 84: summary_stmt_length nit: This now also truncates Query Status. Let's use a more general name like summary_text_length. http://gerrit.cloudera.org:8080/#/c/24353/6/be/src/util/impala-profile-tool.cc@251 PS6, Line 251: nit: use 4-space indent. The current indent is 8-space. http://gerrit.cloudera.org:8080/#/c/24353/6/be/src/util/impala-profile-tool.cc@280 PS6, Line 280: static bool ParseTimestampMicros(const string& timestamp, int64_t* timestamp_us) { I think by using boost::posix_time::time_from_string + boost::posix_time::ptime, this function can be further simplified: if (timestamp.empty()) return false; try { // Boost's time_from_string only handles up to microseconds (6 digits). // Truncate any extra sub-microsecond digits before parsing. string ts = timestamp; size_t dot = ts.find('.', 19); if (dot != string::npos && ts.size() > dot + 7) { ts.resize(dot + 7); } boost::posix_time::ptime t = boost::posix_time::time_from_string(ts); static const boost::posix_time::ptime epoch(boost::gregorian::date(1970, 1, 1)); *timestamp_us = (t - epoch).total_microseconds(); return true; } catch (...) { return false; } Need two more includes: +#include <boost/date_time/posix_time/ptime.hpp> +#include <boost/date_time/posix_time/time_parsers.hpp> We further improve these by letting ParseTimestampMicros return boost::posix_time::ptime and let endTime subtract startTime directly. http://gerrit.cloudera.org:8080/#/c/24353/6/be/src/util/impala-profile-tool.cc@484 PS6, Line 484: return PrettyPrinter::Print(estimated_per_host_mem, TUnit::BYTES); We need to multiply this with the number of hosts. However, do we really need this fallback to handle missing "Cluster Memory Admitted"? This info is added since Impala 3.1 by IMPALA-7349 (around 8 years ago). http://gerrit.cloudera.org:8080/#/c/24353/6/be/src/util/impala-profile-tool.cc@490 PS6, Line 490: if (tree.__isset.exec_summary && tree.exec_summary.__isset.progress : && tree.exec_summary.progress.__isset.num_completed_scan_ranges : && tree.exec_summary.progress.__isset.total_scan_ranges) { : return ProgressToString(tree.exec_summary.progress.num_completed_scan_ranges, : tree.exec_summary.progress.total_scan_ranges); : } I think these are dead code since the progress are only set for live queries. https://github.com/apache/impala/blob/9b0f0686136d8d79af03687898fe3205f81a6700/be/src/service/impala-server.cc#L1080-L1092 When writting the profile log, we just get the TExecSummary: https://github.com/apache/impala/blob/9b0f0686136d8d79af03687898fe3205f81a6700/be/src/service/impala-server.cc#L1109 http://gerrit.cloudera.org:8080/#/c/24353/6/be/src/util/impala-profile-tool.cc@593 PS6, Line 593: query_id As we discussed in the other patch, 'query_id' could be empty string here: https://gerrit.cloudera.org/c/24357/3/be/src/util/impala-profile-tool.cc#544 We should get it from the profile as a fallback, or always getting query id from the profile. http://gerrit.cloudera.org:8080/#/c/24353/6/be/src/util/summary-util-test.cc File be/src/util/summary-util-test.cc: http://gerrit.cloudera.org:8080/#/c/24353/6/be/src/util/summary-util-test.cc@115 PS6, Line 115: Why two spaces here? -- 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: 6 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: Mon, 08 Jun 2026 13:17:34 +0000 Gerrit-HasComments: Yes
