Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/23154 )
Change subject: IMPALA-9846: Enable AGGREGATED PROFILE by Default ...................................................................... Patch Set 35: (5 comments) Sorry for the long silence — replies to your June comments, plus one question on the JSON side. http://gerrit.cloudera.org:8080/#/c/23154/35/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/23154/35/be/src/util/runtime-profile.cc@2112 PS35, Line 2112: if (verbosity <= Verbosity::LEGACY) { PS35 looks right: the verbosity <= LEGACY branch now returns before anything prints total=, and ImpalaServer maps aggregated_profile=false to Verbosity::LEGACY, so the averaged fragment in the traditional profile is back to the plain mean. Thanks for the V1/V2 explanation too — with the averaged fragment being the aggregated representation itself, the deliberate mean/total marker makes sense to me. Resolving; I left a related question on ToJsonImpl below. http://gerrit.cloudera.org:8080/#/c/23154/35/be/src/util/runtime-profile.cc@2118 PS35, Line 2118: bool print_total = !(unit_ == TUnit::TIME_NS || unit_ == TUnit::TIME_US || Rates: fair enough. For instances that really do run concurrently the sum is an aggregate throughput, and I have nothing better to offer, so keeping total there works for me. On the classification: in PS35 print_total still looks only at the TIME_* units and TCounterCategory::AGGREGATED, and AGGREGATED is set in exactly one place — the HighWaterMarkCounter constructor. Next to the ratios you already plan to cover (ExchangeScanRatio is a derived DOUBLE_VALUE ratio), sampling counters look like the same case: both AddSamplingCounter() overloads create a plain Counter through AddCounterLocked(), so AverageThreadTokens stays RAW and gets a summed total even though it is a sampled running average — the category the commit message says should not carry total. Those two overloads are the only place such counters are created, so marking them there may be cheap. Do you plan to do the classification in this change? The goldens here already carry those lines, so landing it later moves them twice. Leaving this open until it's in. http://gerrit.cloudera.org:8080/#/c/23154/35/be/src/util/runtime-profile.cc@2179 PS35, Line 2179: if (verbosity <= Verbosity::LEGACY || stats.num_vals == 1) { Two things drifted apart between this JSON path and PrettyPrintImpl above: - PrettyPrintImpl no longer has the "|| stats.num_vals == 1" half of this condition, so a single-value averaged counter now prints "total=X mean=X min=X max=X" in text while it stays a single "value" here (e.g. RowsSentRate in impala_profile_log_tpcds_compute_stats_v2_default.expected.txt:466). If that is the marker you describe in the total= thread, the JSON side probably wants it as well. - The JSON counter gets no total at all, for any number of values, so a reader of /query_profile_json sees a sum in the text profile that it cannot get from JSON. Both may be deliberate scope for this change — mainly asking whether JSON is meant to follow later. http://gerrit.cloudera.org:8080/#/c/23154/35/tests/common/test_result_verifier.py File tests/common/test_result_verifier.py: http://gerrit.cloudera.org:8080/#/c/23154/35/tests/common/test_result_verifier.py@689 PS35, Line 689: if re.search(r"([^\\]|^)\([^\?]", pattern): Position 0 is covered now, thanks. Two shapes still slip past: a named group "(?P<n>x)", which the [^\?] lets through, and "\\(" — an escaped backslash followed by a group. Both raise the group count and shift the indices this check is here to protect. The pattern is compiled a few lines below anyway — would compiling first and asserting re.compile(pattern).groups == 0 work here? It counts exactly the capturing forms, and non-capturing groups and escaped parens pass on their own. Your call; the case I actually hit is fixed either way. http://gerrit.cloudera.org:8080/#/c/23154/35/tests/common/test_result_verifier.py@716 PS35, Line 716: pattern = r"{0}: total=\d+(?:\.\d+)? ?(?:[KMGB]+)? \((-?\d+(?:\.\d+)?)\)".format(field) Checked pretty-printer.h: GetByteUnit emits only B/KB/MB/GB and GetUnit only K/M/B, so [KMGB]+ covers everything that can show up and TB indeed cannot. Resolving. -- To view, visit http://gerrit.cloudera.org:8080/23154 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If41d6322361fba82c946efd614cc7d28cb1c36e8 Gerrit-Change-Number: 23154 Gerrit-PatchSet: 35 Gerrit-Owner: Surya Hebbar <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Sun, 23 Aug 2026 13:26:10 +0000 Gerrit-HasComments: Yes
