impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1286: Extract common conjuncts from disjunctions.
Date Fri, 28 Oct 2016 17:50:27 GMT
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 <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message