You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org> on 2017/09/27 02:13:29 UTC

[Impala-ASF-CR] IMPALA-4252: Move runtime filters to ScanNode

Thomas Tauber-Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8148


Change subject: IMPALA-4252: Move runtime filters to ScanNode
......................................................................

IMPALA-4252: Move runtime filters to ScanNode

As a preliminary step towards adding runtime filters for Kudu scans,
this patch moves runtime filter related code from HdfsScanNodeBase to
ScanNode so that it's available to KuduScanNodeBase.

Testing:
- Ran existing runtime filter tests.

Change-Id: I17bdc869046dc2cd837d02f333441fa6324ff086
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
4 files changed, 126 insertions(+), 107 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/8148/1
-- 
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: newchange
Gerrit-Change-Id: I17bdc869046dc2cd837d02f333441fa6324ff086
Gerrit-Change-Number: 8148
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>

[Impala-ASF-CR] IMPALA-4252: Move runtime filters to ScanNode

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

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 <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Sep 2017 15:19:59 +0000
Gerrit-HasComments: Yes