Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3823: Add timer to measure Parquet footer reads

Patch Set 2:


Nice! I think this counter may be useful more generally, though I think we 
should rename it (and variable names based on it).
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
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.
File be/src/util/

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>());
vector can be constructed with a size parameter to do this in 1 step.
File common/thrift/RuntimeProfile.thrift:

PS2, Line 56: TMinMaxAvgValueCounter

PS2, Line 59: value

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf87bad90037dd0cea63b10c537382ec0f980cbf
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-HasComments: Yes

Reply via email to