Attila Jeges has posted comments on this change. Change subject: IMPALA-3548: Prune runtime filters based on query options in the FE ......................................................................
Patch Set 2: (8 comments) Thanks for the review. http://gerrit.cloudera.org:8080/#/c/7564/2/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: PS2, Line 119: singleNodePlan > The comment in L448 (RuntimeFilterGenerator) suggests that this function is The single node plan is created on L104 and assigned to 'singleNodePlan'. Distributed plan is created in-place on L114, therefore after L114 'singleNodePlan' refers to the distributed plan. Maybe I should rename 'singleNodePlan' to something else to avoid confusion? PS2, Line 117: // create runtime filters : if (ctx_.getQueryOptions().getRuntime_filter_mode() != TRuntimeFilterMode.OFF) { : RuntimeFilterGenerator.generateRuntimeFilters(ctx_, singleNodePlan); : ctx_.getAnalysisResult().getTimeline().markEvent("Runtime filters computed"); : } > Why was this moved here? The block was moved here because the distributed plan is created on L114 and I wanted to assign runtime filters on the distributed plan instead of the single node plan. Alex suggested this approach. This way we can enforce all the constraints (described in the commit message) during filter assignment directly. http://gerrit.cloudera.org:8080/#/c/7564/2/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java: PS2, Line 142: public boolean isBoundByPartitionColumns = false; : // Indicates if 'node' is in the same fragment as the join that produces the : // filter : public boolean isLocalTarget = false; > can they change once they are set? If not, make them final. Done PS2, Line 165: tFilterTarget.setIs_bound_by_partition_columns(isBoundByPartitionColumns); : tFilterTarget.setIs_local_target(isLocalTarget); > Do you have to send these to the backend? I think they are only used in DCH The BE uses these flags to create instances of Coordinator::FilterTarget that are used in coordinator.cc. PS2, Line 455: Preconditions.checkNotNull(ctx); : Preconditions.checkNotNull(root); > Remove, not really doing anything useful here. Done PS2, Line 548: Preconditions.checkNotNull(ctx); : Preconditions.checkNotNull(scanNode); > remove Done PS2, Line 550: Analyzer analyzer = ctx.getRootAnalyzer(); : boolean isSingleNodeExec = ctx.isSingleNodeExec(); : boolean disableRowRuntimeFiltering = : ctx.getQueryOptions().isDisable_row_runtime_filtering(); : TRuntimeFilterMode runtimeFilterMode = ctx.getQueryOptions().getRuntime_filter_mode(); > nit: move these lines below L557. No need to get these values if we don't e Done http://gerrit.cloudera.org:8080/#/c/7564/2/tests/query_test/test_runtime_filters.py File tests/query_test/test_runtime_filters.py: PS2, Line 87: def test_distrib_plan_filter_pruning(self, vector): : vector.get_value('exec_option')['num_nodes'] = 0 : self.run_test_case('QueryTest/runtime_filters_distrib_pruning', vector) : : def test_singlenode_plan_filter_pruning(self, vector): : vector.get_value('exec_option')['num_nodes'] = 1 : self.run_test_case('QueryTest/runtime_filters_singlenode_pruning', vector) > These should be Planner tests (see PlannerTest.java) These are written as query tests and not planner tests because the planner tests don't have a good way to 'SET' query options on a per-test case basis. (different test cases in 'runtime_filters_distrib_pruning.test' and 'runtime_filters_singlenode_pruning.test' use different query options) -- To view, visit http://gerrit.cloudera.org:8080/7564 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id0f0b200e02442edcad8df3979f652d66c6e52eb Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Attila Jeges <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
