Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/11575 )
Change subject: IMPALA-6964: Track stats about column and page sizes in Parquet reader ...................................................................... Patch Set 12: (6 comments) I had a comment about the lock upgrading, and some nits, but I think it's getting there. http://gerrit.cloudera.org:8080/#/c/11575/12/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: http://gerrit.cloudera.org:8080/#/c/11575/12/be/src/exec/hdfs-scan-node-base.cc@904 PS12, Line 904: bytes_read_per_col_lock_); Please wrap the lock in a scope block to release it after the for() statement ends. http://gerrit.cloudera.org:8080/#/c/11575/12/be/src/exec/hdfs-scan-node-base.cc@962 PS12, Line 962: bytes_read_per_col_guard_read_lock.unlock(); There's a race between unlocking and re-locking here. You can use boost:upgrade_lock to achieve the same effect atomically. http://gerrit.cloudera.org:8080/#/c/11575/12/tests/infra/test_utils.py File tests/infra/test_utils.py: http://gerrit.cloudera.org:8080/#/c/11575/12/tests/infra/test_utils.py@36 PS12, Line 36: def test_get_bytes_summary_stats_counter(): Can you double-check that this actually gets executed? :) http://gerrit.cloudera.org:8080/#/c/11575/12/tests/infra/test_utils.py@37 PS12, Line 37: nit: remove this whitespace http://gerrit.cloudera.org:8080/#/c/11575/12/tests/infra/test_utils.py@40 PS12, Line 40: runtime_profile = "- ExampleCounter: (Avg: 8.00 KB (8192) ; " \ maybe pick an example that's not all the same values? http://gerrit.cloudera.org:8080/#/c/11575/12/tests/util/parse_util.py File tests/util/parse_util.py: http://gerrit.cloudera.org:8080/#/c/11575/12/tests/util/parse_util.py@156 PS12, Line 156: re.X) I think re.VERBOSE is easier to understand -- To view, visit http://gerrit.cloudera.org:8080/11575 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I322f9b324b6828df28e5caf79529085c43d7c817 Gerrit-Change-Number: 11575 Gerrit-PatchSet: 12 Gerrit-Owner: Sahil Takiar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Comment-Date: Tue, 27 Nov 2018 23:18:39 +0000 Gerrit-HasComments: Yes
