Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 )
Change subject: IMPALA-9382: part 1: transposed profile prototype ...................................................................... Patch Set 19: Code-Review+1 (2 comments) I'm basically good to approve this. Unless someone else wants to review, I will bump this to +2. http://gerrit.cloudera.org:8080/#/c/15798/19/be/src/util/runtime-profile-counters.h File be/src/util/runtime-profile-counters.h: http://gerrit.cloudera.org:8080/#/c/15798/19/be/src/util/runtime-profile-counters.h@475 PS19, Line 475: RuntimeProfile::Counter I think I would prefer it to inherit from RuntimeProfileBase::Counter, since SummaryStatsCounter is a class declared in RuntimeProfileBase. http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@1892 PS17, Line 1892: counter->total_num_values[i] = counter->total > Yeah I was trying to minimize disruption to the rest of the codebase. We co I agree that we don't want to disrupt the rest of the codebase. I'm thinking more about a few mismatches of the declaration vs the definition in runtime-profile.h/runtime-profile.cc/runtime-profile-counters.h. For example, we declare RuntimeProfileBase::AveragedCounter, but we define the implementations of its functions with RuntimeProfile::AveragedCounter. The compiler can figure it out, and it is not a big deal, but I think it is slightly clearer for the definition to match the declaration. Specifically, if the class is declared in RuntimeProfileBase, then we use that for all its function definitions (even if it has an alias in RuntimeProfile). So, the SummaryStatsCounter definitions are RuntimeProfileBase::SummaryStatsCounter rather than RuntimeProfile::SummaryStatsCounter. I think this would only impact Counter, SummaryStatsCounter, and AveragedCounter, and it shouldn't impact anything outside the files we are already touching. None of this would impact the classes declared in RuntimeProfile. Does that make sense? -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 19 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 10 Sep 2020 23:54:39 +0000 Gerrit-HasComments: Yes
