Tim Armstrong 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 1: (5 comments) 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: nit: i instead of j since we unnested the loop 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: : > Currently I added the header checking functionality to BaseSequenceScanner, The approaches seem pretty comparable in readability. I guess the major advantage of putting it in ScanRangeMetadata is that we don't need to instantiate the scanner before checking the filters, which could be moderately expensive (although not a show stopper). So I think I have a slight preference for that approach. 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: runtime_state_, this, partition, scan_range, filter_ctxs, expr_results_pool); nit: we could probably just put this expression inline in the if() right? Not sure that pulling it out makes it more readable. http://gerrit.cloudera.org:8080/#/c/8684/2/be/src/exec/hdfs-scan-node.cc@518 PS2, Line 518: nit: bs_scanner != nullptr, just to be explicit. http://gerrit.cloudera.org:8080/#/c/8684/2/be/src/exec/hdfs-scan-node.cc@519 PS2, Line 519: if (VLOG_QUERY_IS_ON) { nit: unnecessary vertical whitespace -- 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: 1 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: Fri, 01 Dec 2017 19:37:51 +0000 Gerrit-HasComments: Yes
