Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8684 )
Change subject: IMPALA-3804: Re-enable per-scan filtering for sequence-based scanners ...................................................................... Patch Set 2: (5 comments) Thanks! http://gerrit.cloudera.org:8080/#/c/8684/2/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: http://gerrit.cloudera.org:8080/#/c/8684/2/be/src/exec/hdfs-scan-node-base.cc@691 PS2, Line 691: for (int j = 0; j < file->splits.size(); ++j) { > nit: i instead of j since we unnested the loop Done http://gerrit.cloudera.org:8080/#/c/8684/1/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/8684/1/be/src/exec/hdfs-scan-node.cc@a497 PS1, Line 497: : > The approaches seem pretty comparable in readability. I guess the major adv OK, I did that in the new commit. Turned out it doesn't require much more code modifications. http://gerrit.cloudera.org:8080/#/c/8684/2/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/8684/2/be/src/exec/hdfs-scan-node.cc@512 PS2, Line 512: bool range_is_filtered = !PartitionPassesFilters( > nit: we could probably just put this expression inline in the if() right? N Done http://gerrit.cloudera.org:8080/#/c/8684/2/be/src/exec/hdfs-scan-node.cc@518 PS2, Line 518: bs_scanner > nit: bs_scanner != nullptr, just to be explicit. Not applicable with the ScanRangeMetadata::is_sequence_header approach http://gerrit.cloudera.org:8080/#/c/8684/2/be/src/exec/hdfs-scan-node.cc@519 PS2, Line 519: > nit: unnecessary vertical whitespace The new code is restructured, now these checks happen in their original place. -- To view, visit http://gerrit.cloudera.org:8080/8684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b38c26bcbe67f83efcc65a1723d766626ae3d3e Gerrit-Change-Number: 8684 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 04 Dec 2017 13:57:22 +0000 Gerrit-HasComments: Yes
