Dan Hecht has posted comments on this change. Change subject: IMPALA-5210: Count rows and collection items in parquet scanner separately ......................................................................
Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/7776/5/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: PS5, Line 963: coll_items_read_counter_ > If we pass it through function calls about 10 function signatures would be You could make that argument for all parameters, but it's not a good pattern to follow in general. That said, given the way our counters currently work, I'm okay with having this thread-local counter live in the scanner object. But I think it would help clarify it if we follow a slightly different pattern to accumulate the thread-local counter in the the global counter: COUNTER_ADD(global_counter, thread_local_counter_); thread_local_counter_ = 0; // Was accounted into global_counter. i.e. move this line down to after line 1010. That way it's clearer that no counts are ever lost or double counted, etc. It would also be nice to explain concisely why we have this thread local counter in its header comment. -- To view, visit http://gerrit.cloudera.org:8080/7776 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7f6efddaea18507482940f5bdab7326b6482b067 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tianyi Wang <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-HasComments: Yes
