Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-1286: Extract common conjuncts from disjunctions. ......................................................................
Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/4877/1/fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java File fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java: Line 42: if (!Expr.IS_OR_PREDICATE.apply(expr)) return expr; Isn't this check a little weak in the sense that it only checks the root's Op type to be OR. IIUC, a case like ((a OR b) OR (a AND c)) still passes and returns 'a' as the common conjunct. Is it better to have a check that makes sure the expr is in a proper disjunctive normal form? http://gerrit.cloudera.org:8080/#/c/4877/1/testdata/workloads/functional-planner/queries/PlannerTest/tpch-all.test File testdata/workloads/functional-planner/queries/PlannerTest/tpch-all.test: Line 3384: p_brand = 'Brand#12' I'm not really sure how this patch handles a bushy predicate. Do we flatten them somewhere and convert it to a DNF before applying the new rule? (May be I misread the code) For example: ((a or (b and c)) or ((a and b) or (d or f))) The tests here and elsewhere seem to be handling simple predicates, so I'm wondering how cases like above are dealt with. -- To view, visit http://gerrit.cloudera.org:8080/4877 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3cf9b950afaa3fd753d1b09ba5e540b5258940ad Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
