Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8148 )
Change subject: IMPALA-4252: Move runtime filters to ScanNode ...................................................................... Patch Set 1: (4 comments) Looks fine, just very minor comments. It looks like there weren't really any changes to the code aside from the move and method signature change. http://gerrit.cloudera.org:8080/#/c/8148/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8148/1//COMMIT_MSG@12 PS1, Line 12: Was the move totally mechanical? It would be good to mention if there were any changes required to the logic that need closer scrutiny. http://gerrit.cloudera.org:8080/#/c/8148/1/be/src/exec/scan-node.h File be/src/exec/scan-node.h: http://gerrit.cloudera.org:8080/#/c/8148/1/be/src/exec/scan-node.h@195 PS1, Line 195: time_ms Comment needs an update - this is no longer an argument. http://gerrit.cloudera.org:8080/#/c/8148/1/be/src/exec/scan-node.h@198 PS1, Line 198: RuntimeState* state Both HdfsScanNodeBase and KuduScanNodeBase have a "RuntimeState* runtime_state_" member. Might be cleaner to just move that member to ScanNode and avoid the inconsistency of passing RuntimeState* via arguments some of the time. http://gerrit.cloudera.org:8080/#/c/8148/1/be/src/exec/scan-node.cc File be/src/exec/scan-node.cc: http://gerrit.cloudera.org:8080/#/c/8148/1/be/src/exec/scan-node.cc@79 PS1, Line 79: // TODO: Move this to Prepare() Does this comment still make sense? I wish whoever added it had mentioned why it wasn't already moved to Prepare(). -- To view, visit http://gerrit.cloudera.org:8080/8148 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I17bdc869046dc2cd837d02f333441fa6324ff086 Gerrit-Change-Number: 8148 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 27 Sep 2017 15:19:59 +0000 Gerrit-HasComments: Yes
