Tim Armstrong has posted comments on this change. Change subject: IMPALA-4863: Correctly account the file type and compression codec ......................................................................
Patch Set 2: (6 comments) We should add a regression test for this as well. http://gerrit.cloudera.org:8080/#/c/7245/2//COMMIT_MSG Commit Message: Line 7: IMPALA-4863: Correctly account the file type and compression codec It would be easier to review if we included the IMPALA-5311 fix in this, since they will require related changes to the same code path. http://gerrit.cloudera.org:8080/#/c/7245/1/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: PS1, Line 771: const const is redundant. Should also have a space after "," http://gerrit.cloudera.org:8080/#/c/7245/2/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: PS2, Line 553: true It seems like this will remove useful information for file formats where we *do* know the compression codec ahead of time. I think we should rethink the parameters here. If I understand correctly there are three outcomes: * File is not filtered * File is filtered but is a file format where we know the compression * File is filtered but is a file format where we don't know the compression without scanning it. Line 771: const THdfsCompression::type& compression_type,const bool filtered) { missing space - maybe run it it through clang-format before posting? Also remove the "const", it doesn't make much sense for arguments passed by value. http://gerrit.cloudera.org:8080/#/c/7245/2/be/src/exec/hdfs-scan-node-base.h File be/src/exec/hdfs-scan-node-base.h: PS2, Line 250: =f should have spaces around " = " PS2, Line 462: don't need space between >> in C++11 -- To view, visit http://gerrit.cloudera.org:8080/7245 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I797916505f62e568f4159e07099481b8ff571da2 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: anujphadke <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-HasComments: Yes
