Tianyi Wang has posted comments on this change. Change subject: IMPALA-5210: Count rows and collection items in parquet scanner separately ......................................................................
Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/7776/2/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: Line 964: while (!column_readers[0]->RowGroupAtEnd()) { > Yeah, it changes the semantics to something like "Number of successfully co Done http://gerrit.cloudera.org:8080/#/c/7776/3/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: Line 1008: COUNTER_ADD(scan_node_->rows_read_counter(), num_rows_read); > You could use COUNTER_SET here and get rid of num_rows_read altogether. I'm not sure what value should be COUNTER_SET here. Could you elaborate? PS3, Line 1009: COUNTER_ADD > How about removing the initialization at the beginning of the method and us Same as above. http://gerrit.cloudera.org:8080/#/c/7776/3/be/src/exec/hdfs-parquet-scanner.h File be/src/exec/hdfs-parquet-scanner.h: Line 470: /// Number of collection items read > Can you describe the scope of this in the comment, e.g. "Total number of co Updated. In current code it's total number of collection items read in current row batch. Do you suggest let it count throughout the lifetime of this scanner and update the scannode counter when this scanner is cleaned up? http://gerrit.cloudera.org:8080/#/c/7776/3/be/src/exec/scan-node.h File be/src/exec/scan-node.h: Line 153: /// # collection items read from the scanner > Can you explain here that this is across nested collections, so that A = {B Done -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tianyi Wang <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-HasComments: Yes
