Yongjun Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11379 )
Change subject: IMPALA-6442: Misleading file offset reporting in error messages. ...................................................................... Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/11379/1/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: http://gerrit.cloudera.org:8080/#/c/11379/1/be/src/exec/hdfs-parquet-scanner.cc@1165 PS1, Line 1165: sca > Now that there are two variables storing some kind of length it would be go Done in new rev. http://gerrit.cloudera.org:8080/#/c/11379/1/be/src/exec/hdfs-parquet-scanner.cc@1214 PS1, Line 1214: // file_len - 4-byte footer length field - 4-byte version number field - metadata size : int64_t metadata_start = file_len - sizeof(int32_t) - > not your change but can you please fix the line wrapping while you're here? Done in new rev. http://gerrit.cloudera.org:8080/#/c/11379/1/be/src/exec/hdfs-parquet-scanner.cc@1228 PS1, Line 1228: const HdfsFileDesc* file_desc = scan_node_->GetFileDesc(partition_id, filename()); > If both are NULL we won't catch it now. Are we sure that cannot happen? Sho Thanks a lot for the review Lars. The assumption is that stream_->file_desc() is not null, if it's null, the call to get file length before this branch would have failed already. If we want, we could add a DCHECK in the beginning of the function. I added the DCHECK here for multiple reasons: 1. only the logic in the existing code here tries to find file_desc. 2. this branch is an "unlikely" branch. Code out side this branch has no need to find file_desc. 3. the partition_id variable is only used in this branch. Another version of my code did move these few lines to before this branch, but I chose to move it here because otherwise it's going to be some additional work for other code that doesn't need to reach this "unlikely" branch and doesn't use the partition_id variable. What do you think? http://gerrit.cloudera.org:8080/#/c/11379/1/be/src/exec/hdfs-parquet-scanner.cc@1267 PS1, Line 1267: "at file of > nit: wrap after file_length done in new rev. http://gerrit.cloudera.org:8080/#/c/11379/1/testdata/data/corrupt_footer_len_decr.parquet File testdata/data/corrupt_footer_len_decr.parquet: http://gerrit.cloudera.org:8080/#/c/11379/1/testdata/data/corrupt_footer_len_decr.parquet@1 PS1, Line 1: PAR1L Ò , &n %c^f&6& 6 (Ò Ò , ,Hschema %c%$ &n %c^f&6& 6 (Ò Ò , f (Oimpala version 2.11.0-SNAPSHOT (build 26b2cc85d2084e9e464669b2a67a8015ede173e1) ½ PAR1 > Add a description for both test files to the README file in the same folder done in new rev. http://gerrit.cloudera.org:8080/#/c/11379/1/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: http://gerrit.cloudera.org:8080/#/c/11379/1/tests/query_test/test_scanners.py@401 PS1, Line 401: def corrupt_footer_len_common(self, vector, unique_database, testname_postfix): > The function should have a comment. Please find a more descriptive name for Done in new rev. http://gerrit.cloudera.org:8080/#/c/11379/1/tests/query_test/test_scanners.py@412 PS1, Line 412: create_table_and_copy_files(self.client, > nit: single line Done in new rev. http://gerrit.cloudera.org:8080/#/c/11379/1/tests/query_test/test_scanners.py@420 PS1, Line 420: > nit: capitalization Done in new rev. -- To view, visit http://gerrit.cloudera.org:8080/11379 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I35235e99ea9ceb0d31961dd3b8069f7194f5a2de Gerrit-Change-Number: 11379 Gerrit-PatchSet: 2 Gerrit-Owner: Yongjun Zhang <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Yongjun Zhang <[email protected]> Gerrit-Comment-Date: Thu, 06 Sep 2018 08:11:06 +0000 Gerrit-HasComments: Yes
