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 <tmarsh...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-HasComments: Yes
