impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5210: Count rows and collection items in parquet scanner separately
Date Wed, 30 Aug 2017 19:10:17 GMT
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 <twang@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <twang@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message