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

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8148 )

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


Patch Set 2: Verified+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: comment
Gerrit-Change-Id: I17bdc869046dc2cd837d02f333441fa6324ff086
Gerrit-Change-Number: 8148
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 02 Oct 2017 20:01:24 +
Gerrit-HasComments: No

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

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
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.

The change was mechanical with no logic changes, except for moving the
calculation of the max wait time into WaitForRuntimeFilters().

Testing:
- Ran existing runtime filter tests.

Change-Id: I17bdc869046dc2cd837d02f333441fa6324ff086
Reviewed-on: http://gerrit.cloudera.org:8080/8148
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/kudu-scan-node-base.cc
M be/src/exec/kudu-scan-node-base.h
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
6 files changed, 131 insertions(+), 114 deletions(-)

Approvals:
  Tim Armstrong: Looks good to me, approved
  Impala Public Jenkins: Verified

--
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: merged
Gerrit-Change-Id: I17bdc869046dc2cd837d02f333441fa6324ff086
Gerrit-Change-Number: 8148
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

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

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8148 )

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


Patch Set 2:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1284/


--
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: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 02 Oct 2017 16:00:26 +
Gerrit-HasComments: No

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

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8148 )

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


Patch Set 1:

(1 comment)

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()
> Not sure, it was left by Michael (I'll ping him about it) in the review "Di
Yes, I should have probably left more details. The original plan for the MT 
work is to have one filter bank shared across all fragment instances. The 
single PlanNode instance of ScanNode will then register the filter during the 
PlanNode tree creation. Then, during ScanNode::Prepare() for an ExecTree 
instance (inside a fragment instance), we will just look up the filter in the 
shared filter bank and fill in the filter_ctxs_.



--
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 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 02 Oct 2017 06:44:47 +
Gerrit-HasComments: Yes

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

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8148 )

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


Patch Set 2:

(3 comments)

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: Scanner
> Comment needs an update - this is no longer an argument.
Done


http://gerrit.cloudera.org:8080/#/c/8148/1/be/src/exec/scan-node.h@198
PS1, Line 198: rs to arrive, check
> Both HdfsScanNodeBase and KuduScanNodeBase have a "RuntimeState* runtime_st
Done


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 w
Not sure, it was left by Michael (I'll ping him about it) in the review 
"Disentangle Expr and ExprContext"

You asked about it during that review too, and his response was "I am thinking 
of moving it when PlanNode is introduced." but I'm not sure what that means.



--
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: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:34:40 +
Gerrit-HasComments: Yes

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

[Thomas Tauber-Marshall (Code Review)]

Hello Tim Armstrong,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8148

to look at the new patch set (#2).

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.

The change was mechanical with no logic changes, except for moving the
calculation of the max wait time into WaitForRuntimeFilters().

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/kudu-scan-node-base.cc
M be/src/exec/kudu-scan-node-base.h
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
6 files changed, 131 insertions(+), 114 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/8148/2
--
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: newpatchset
Gerrit-Change-Id: I17bdc869046dc2cd837d02f333441fa6324ff086
Gerrit-Change-Number: 8148
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

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

[Tim Armstrong (Code Review)]

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 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 27 Sep 2017 15:19:59 +
Gerrit-HasComments: Yes

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

[Thomas Tauber-Marshall (Code Review)]

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