Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16474 )
Change subject: IMPALA-10178 Run-time profile shall report skews ...................................................................... Patch Set 41: Code-Review+1 (6 comments) mostly nits, otherwise approach LGTM. @Tim if you want to take another look as well. I think the patch has changed significantly over the past few weeks. http://gerrit.cloudera.org:8080/#/c/16474/41/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/16474/41/be/src/service/query-options.cc@998 PS41, Line 998: } : if (set_query_options_mask != NULL) { : DCHECK_LT(option, set_query_options_mask->size()); : set_query_options_mask->set(option); : } unnecessary change http://gerrit.cloudera.org:8080/#/c/16474/41/be/src/util/runtime-profile-counters.h File be/src/util/runtime-profile-counters.h: http://gerrit.cloudera.org:8080/#/c/16474/41/be/src/util/runtime-profile-counters.h@488 PS41, Line 488: bool EvaluateSkewWithCoV(double threshold, std::stringstream* details); nit: document return value http://gerrit.cloudera.org:8080/#/c/16474/41/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: http://gerrit.cloudera.org:8080/#/c/16474/41/be/src/util/runtime-profile.h@203 PS41, Line 203: recurssively nit: typo http://gerrit.cloudera.org:8080/#/c/16474/41/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/16474/41/be/src/util/runtime-profile.cc@675 PS41, Line 675: Each spec is a pair of a profile name prefix and a list of : // counter names. would be nice to mention that all counters *have* to be averaged counters http://gerrit.cloudera.org:8080/#/c/16474/41/be/src/util/runtime-profile.cc@677 PS41, Line 677: static const unordered_map<string, vector<string>> skew_profile_specs = { : {"KUDU_SCAN_NODE", {"RowsRead"}}, {"HDFS_SCAN_NODE", {"RowsRead"}}, : {"HASH_JOIN_NODE", {"ProbeRows", "BuildRows"}}, : {"GroupingAggregator", {"RowsReturned"}}, {"EXCHANGE_NODE", {"RowsReturned"}}, : {"SORT_NODE", {"RowsReturned"}}}; nit: would be nice to define a struct that encapsulates the entries in the map. something like struct SkewProfileSpec { string node_name; vector<string> counter_names; }; http://gerrit.cloudera.org:8080/#/c/16474/41/be/src/util/runtime-profile.cc@1960 PS41, Line 1960: if (remove_last_comma) { : ss.seekp(-1, std::ios_base::end); : } you can probably simplify the string manipulation logic by using something like boost::algorithm::join - https://stackoverflow.com/questions/1833447/a-good-example-for-boostalgorithmjoin -- To view, visit http://gerrit.cloudera.org:8080/16474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I91041f2856eef8293ea78f1721f97469062589a1 Gerrit-Change-Number: 16474 Gerrit-PatchSet: 41 Gerrit-Owner: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 12 Oct 2020 17:42:07 +0000 Gerrit-HasComments: Yes
