[Impala-ASF-CR] IMPALA-3823: Add timer to measure Parquet footer reads

["Matthew Jacobs (Code Review)" <ge...@cloudera.org>]

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 <sa...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes