Tim Armstrong 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 3: (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/tpcds-all.test so we're testing this directly. Did any of the existing planner tests change at all? 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: * it can work for single table predicates also (primarily intended for testing). The single table rewrites might be useful for pushing predicates down into the storage (a lot of optimisations like min-max filtering or pushdown into Kudu won't work for OR conjuncts). I think this is OK as-is but maybe worth thinking about as an extension. http://gerrit.cloudera.org:8080/#/c/15462/3/fe/src/main/java/org/apache/impala/rewrite/ConvertToCNFRule.java@132 PS3, Line 132: } else if (rhs instanceof CompoundPredicate && Consider factoring the logic of the two branches out into a common function, since it's essentialyl the same with lhs and rhs swapped (although I guess the order of the expressions in the output is slightly different, so OK to ignore if that was your intent.) http://gerrit.cloudera.org:8080/#/c/15462/3/fe/src/main/java/org/apache/impala/rewrite/ConvertToCNFRule.java@136 PS3, Line 136: List<Expr> disjuncts = new ArrayList<>(); I think it would be more concise to use Arrays.asList() to construct the list. That would make it significantly more readable IMO by reducing the visual noise. http://gerrit.cloudera.org:8080/#/c/15462/3/fe/src/main/java/org/apache/impala/rewrite/ConvertToCNFRule.java@156 PS3, Line 156: if (forMultiTablesOnly_) { It would be good to factor this check out into a common function. -- 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: 3 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: Tue, 17 Mar 2020 22:58:30 +0000 Gerrit-HasComments: Yes
