impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1286: Extract common conjuncts from disjunctions.
Date Wed, 02 Nov 2016 17:34:38 GMT
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-1286: Extract common conjuncts from disjunctions.
......................................................................


Patch Set 2: Code-Review+2

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4877/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:

Line 70:         BetweenToCompoundRule.INSTANCE, ExtractCommonConjunctRule.INSTANCE);
for added effectiveness you want the between rule to be applied before the common-conjuncts
rule.

point this out in a comment.


http://gerrit.cloudera.org:8080/#/c/4877/2/fe/src/main/java/org/apache/impala/analysis/Subquery.java
File fe/src/main/java/org/apache/impala/analysis/Subquery.java:

Line 155:     return stmt_.toSql().equals(((Subquery)o).stmt_.toSql());
hm, that seems a bit fragile, won't insignificant syntactic differences (such as extra parentheses)
throw this off?

i guess we're missing a QueryStmt.equals(), and it's too ambitious for this patch, but please
leave a todo.


http://gerrit.cloudera.org:8080/#/c/4877/2/fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
File fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java:

Line 51:     List<Expr> child0Conjuncts = expr.getChild(0).getConjuncts();
checkstate that the child conjuncts aren't empty?

what about "pathological" clauses like (true or b)?


Line 62:         conjunct.setPrintSqlInParens(false);
add comment: why


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message