Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/16912 )
Change subject: IMPALA-10412: ConvertToCNFRule can be apply to view table ...................................................................... Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/16912/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16912/5//COMMIT_MSG@7 PS5, Line 7: apply to view table : nit: 'applied to a view' http://gerrit.cloudera.org:8080/#/c/16912/5//COMMIT_MSG@9 PS5, Line 9: if the predicate associated more than one tuples and the predicate can This sentence seems to be a continuation of the title summary. In general, it is preferred to have the full independent description here. Something like...'For OR predicates that reference a view, currently the ConvertToCNFRule does not get applied since it is considered a single table predicate even if the predicate might reference columns from different tables within the view. This patch enables the application of this rule for such predicates by checking the expanded view and if it satisfies the criterion then the rule can be applied and the predicate can be pushed eventually to the scan. http://gerrit.cloudera.org:8080/#/c/16912/5//COMMIT_MSG@13 PS5, Line 13: Add nit: 'Added' http://gerrit.cloudera.org:8080/#/c/16912/5/fe/src/main/java/org/apache/impala/rewrite/ConvertToCNFRule.java File fe/src/main/java/org/apache/impala/rewrite/ConvertToCNFRule.java: http://gerrit.cloudera.org:8080/#/c/16912/5/fe/src/main/java/org/apache/impala/rewrite/ConvertToCNFRule.java@122 PS5, Line 122: TableRef tbl = analyzer.getTableRef(tids.get(0)); tids.get(0) could hit index out of bounds if tids was empty .. which is a corner case but since the if condition above allows size <=1, it is still possible. You can move this logic outside this check and just enclose it within a : 'if (tids.size() == 1)' check. http://gerrit.cloudera.org:8080/#/c/16912/5/fe/src/main/java/org/apache/impala/rewrite/ConvertToCNFRule.java@124 PS5, Line 124: Expr basePred = pred.trySubstitute(((InlineViewRef)tbl).getBaseTblSmap(), I think this should use 'cpred' instead of pred since cpred is the the analyzed (and cloned) version .. see above on line 114. http://gerrit.cloudera.org:8080/#/c/16912/5/fe/src/main/java/org/apache/impala/rewrite/ConvertToCNFRule.java@126 PS5, Line 126: tids.clear(); If the predicate did not change based on the previous trySubstitute() call, then it is better to skip clearing/re-populating the tids. http://gerrit.cloudera.org:8080/#/c/16912/5/testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test File testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test: http://gerrit.cloudera.org:8080/#/c/16912/5/testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test@2487 PS5, Line 2487: apply nit: 'applied' http://gerrit.cloudera.org:8080/#/c/16912/5/testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test@2491 PS5, Line 2491: select count(*) from t where t.test_zip = 1 or (t.test_name='xyz' and t.zip=1); Could you also add a test where the inline view itself is joined with another table ? e.g select count(*) from t, functional.alltypes other_table where t.test_id = other_table.id AND t.test_zip = 1 or (t.test_name='xyz' and t.zip=1) ; In this case, you would expect the CNF to be applied http://gerrit.cloudera.org:8080/#/c/16912/5/testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test@2515 PS5, Line 2515: # IMPALA-10412: ConvertToCNFRule can be apply to view table It wasn't clear what the purpose of this test was. If it is not really exercising the rule, you could skip it. -- To view, visit http://gerrit.cloudera.org:8080/16912 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie7a9a215d6b92aec07153e643268370f34186c88 Gerrit-Change-Number: 16912 Gerrit-PatchSet: 5 Gerrit-Owner: Xiaoqing Gao <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Sat, 02 Jan 2021 07:13:30 +0000 Gerrit-HasComments: Yes
