Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14234 )
Change subject: IMPALA-7637: Add more hash table stats to profile ...................................................................... Patch Set 1: (2 comments) Looks good, I had a couple of requests for cleanup that were problems with the pre-existing code. http://gerrit.cloudera.org:8080/#/c/14234/1/be/src/exec/hash-table.h File be/src/exec/hash-table.h: http://gerrit.cloudera.org:8080/#/c/14234/1/be/src/exec/hash-table.h@741 PS1, Line 741: int64_t NumHashCollisions() const { return num_hash_collisions_; } nit: we usually use lower_case for trivial accessor functions (this is allowed by the google style guide - see https://google.github.io/styleguide/cppguide.html#Function_Names) This was a pre-existing pattern in this file, but I think now is a good time to make it consistent with the rest of the codebase. http://gerrit.cloudera.org:8080/#/c/14234/1/be/src/exec/partitioned-hash-join-builder.cc File be/src/exec/partitioned-hash-join-builder.cc: http://gerrit.cloudera.org:8080/#/c/14234/1/be/src/exec/partitioned-hash-join-builder.cc@595 PS1, Line 595: void PhjBuilder::AddHashTableStatsToProfile(RuntimeProfile *profile) { The HashTable class is also used in the hash aggregation (grouping-aggregator.cc), so we should try to also include the stats there too. I think it would make more sense to have helper methods in HashTable to create and update the profile counters, so that it can be easily reused. It looks like this pattern of having counters in the hash join but not the agg was already there, but it would be really nice to improve it at this point. -- To view, visit http://gerrit.cloudera.org:8080/14234 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1fd875dd1af8031242fd5f5ff554d3a71aaa6f87 Gerrit-Change-Number: 14234 Gerrit-PatchSet: 1 Gerrit-Owner: Yongzhi Chen <yc...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com> Gerrit-Comment-Date: Mon, 16 Sep 2019 16:19:54 +0000 Gerrit-HasComments: Yes
