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-1861: Simplify conditionals with constant conditions
Date Wed, 11 Jan 2017 22:54:34 GMT
Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-1861: Simplify conditionals with constant conditions
......................................................................


Patch Set 6:

(5 comments)

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

Line 82:       rules.add(SimplifyConditionalsRule.INSTANCE);
Given these rules can get complex over time, I think we should track the dependencies as a
DAG or something just to make sure we get the order right. Else there is a chance we can get
nasty bugs. Its out of scope for this patch but thats my opinion for further changes.


http://gerrit.cloudera.org:8080/#/c/5585/6/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:

Line 73:     if (fnName.getFunction().equals("if")) {
May be add a Preconditions check on expr.getChildren().size() == 2?


Line 127:     return expr;
Unreachable? If yes add a Preconditions.checkState(false)?


Line 155:     for (int i = loopStart; i < numChildren - 1; i += 2) {
Can't we merge this with the above loop? Is it written this way for readability?


Line 175:       if (whenExpr instanceof BoolLiteral) {
I have a question around this. Lets say we have something like

CASE
WHEN a=1 return foo
WHEN TRUE return bar
...

Lets say we do have an 'a' which is 1. Aren't we supposed to evaluate those WHENs in the same
order? If we apply this rule, we replace the whole thing with bar? Without this patch, we'd
return foo. No? (same applies for FALSE)


-- 
To view, visit http://gerrit.cloudera.org:8080/5585
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id70aaf9fd99f64bd98175b7e2dbba28f350e7d3b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tmarshall@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: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message