Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/15462 )
Change subject: IMPALA-9183: Convert disjunctive predicates to conjunctive normal form ...................................................................... Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/15462/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15462/3//COMMIT_MSG@35 PS3, Line 35: Testing: > Maybe add Q13 to testdata/workloads/functional-planner/queries/PlannerTest/ I have added tpcds q13 to the tpcds-all.test and a duplicate q7, q19 to tpch-all.test. These qualify for the rewrite and all have a QUERYOPTIONS section with cnf rewrite enabled. For tpcds q13, there was no previous entry in tpcds-all.test (I see only 24 queries in that file out of 99 total, so the 'all' is misleading). Since the rewrite is disabled by default (at least temporarily), other tests in PlannerTest are not affected. However, when I ran it with it globally enabled, 5 out of 102 tests failed. Most of these were related to TPC-H q7, TPCH-q19 since they are run against different databases - e.g tpch_nested_parquet, tpch_kudu etc. Once notable change in the latest patch upload: I changed the default max_cnf_exprs to 0 (unlimited) because the above tests do generate hundreds of exprs. I cannot think of a good default at this time. If you think we should limit it by default, let me know. http://gerrit.cloudera.org:8080/#/c/15462/3/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/15462/3/fe/src/main/java/org/apache/impala/rewrite/ConvertToCNFRule.java@37 PS3, Line 37: * > The single table rewrites might be useful for pushing predicates down into Agreed. I have removed the wording 'primarily intended for testing.' The default is still multi tables because otherwise we would see a lot of plan diffs. http://gerrit.cloudera.org:8080/#/c/15462/3/fe/src/main/java/org/apache/impala/rewrite/ConvertToCNFRule.java@132 PS3, Line 132: } else if (cpred.getOp() == CompoundPredicate.Operator.NOT) { > Consider factoring the logic of the two branches out into a common function Yes, my intent was to maintain the ordering of applying the distributive property which is left-to-right. I was able to factor out the code though, so it is moved to a separate function. http://gerrit.cloudera.org:8080/#/c/15462/3/fe/src/main/java/org/apache/impala/rewrite/ConvertToCNFRule.java@136 PS3, Line 136: // predicate: NOT (a OR b) > I think it would be more concise to use Arrays.asList() to construct the li Done. http://gerrit.cloudera.org:8080/#/c/15462/3/fe/src/main/java/org/apache/impala/rewrite/ConvertToCNFRule.java@156 PS3, Line 156: private Predicate createPredAndIncrementCount(Expr first_lhs, Expr second_lhs, > It would be good to factor this check out into a common function. Moved this code up so that it is common to both the OR and the NOT blocks. -- To view, visit http://gerrit.cloudera.org:8080/15462 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5a03cd7239333aaf375416ef5f2b7608fcd4a072 Gerrit-Change-Number: 15462 Gerrit-PatchSet: 4 Gerrit-Owner: Aman Sinha <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 18 Mar 2020 23:28:40 +0000 Gerrit-HasComments: Yes
