Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3823: Add timer to measure Parquet footer reads ......................................................................
Patch Set 2: (16 comments) Nice! I think this counter may be useful more generally, though I think we should rename it (and variable names based on it). http://gerrit.cloudera.org:8080/#/c/4371/2//COMMIT_MSG Commit Message: PS2, Line 15: a remove a PS2, Line 18: This is also displayed in the RuntimeProfile. remove, this is implied if it is a runtime profile counter PS2, Line 21: move this new MinMaxAvgValueCounter between nodes through Thrift serialize ... to thrift http://gerrit.cloudera.org:8080/#/c/4371/2/be/src/util/runtime-profile-counters.h File be/src/util/runtime-profile-counters.h: PS2, Line 214: MinMaxAvgValueCounter I'd be nice to have a simpler name rather than enumerate the stats this keeps, e.g. SummaryStatsCounter PS2, Line 223: } : M space between these PS2, Line 226: total_num_values_(0), : current_sum_(0) initialize min and max as well PS2, Line 230: int64_t min_value() const { : return current_min_; : } 1 line PS2, Line 234: int64_t max_value() const { : return current_max_; : } 1 line PS2, Line 238: /// Update current_sum_ with the new value. And also update the min and the max number : /// seen so far. this can be one sentence PS2, Line 240: Counter* new_counter This is a bit confusing since it starts to look like the AveragedCounter which actually keeps Counter*s. I think this should just take the raw values since it doesn't really matter whether or not the new values come from another counter or not. It'd also make this more useful more generally if the source isn't another counter. PS2, Line 244: virtual void Set(double value) { : DCHECK(false); : } : : virtual void Set(int64_t value) { : DCHECK(false); : } : : virtual void Add(int64_t delta) { : DCHECK(false); : } remove extra space, each fn can be 1 line. I know this is what happens above in AveragedCounter, and it'd be nice to remove the extra space there too. http://gerrit.cloudera.org:8080/#/c/4371/2/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: PS2, Line 303: if (it == min_max_avg_value_counter_map_.end()) { if this counter already existed, shouldn't the values be updated? Line 661: { can you add a comment with how this looks printed out? PS2, Line 794: vector<TMinMaxAvgValueCounter>()); : node.min_max_avg_value_counters.resize(min_max_avg_value_counter_map_.size()); vector can be constructed with a size parameter to do this in 1 step. http://gerrit.cloudera.org:8080/#/c/4371/2/common/thrift/RuntimeProfile.thrift File common/thrift/RuntimeProfile.thrift: PS2, Line 56: TMinMaxAvgValueCounter TSummaryStatsCounter PS2, Line 59: value sum? -- To view, visit http://gerrit.cloudera.org:8080/4371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icf87bad90037dd0cea63b10c537382ec0f980cbf Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-HasComments: Yes