Thomas Tauber-Marshall has posted comments on this change. Change subject: IMPALA-1861: Simplify conditionals with constant conditions ......................................................................
Patch Set 7: (6 comments) 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? Done Line 127: > Unreachable? If yes add a Preconditions.checkState(false)? No, it would reach this for "NOT" on a non-constant expression. Line 155: // CASE expr while removing any FALSE or NULL cases. > Can't we merge this with the above loop? Is it written this way for readabi This was done for performance, since these rules may be executed repeatedly, the above loop is cheaper than this one, and I'm assuming that CASEs that can't be simplified is the common case, though I don't think it makes a huge difference. Line 157: Expr elseExpr = null; > single line Done Line 175: > Given the complexity of some of the case/when clauses we see in practice I' Per Greg's comments on the mailing list, we're going to go with the ANSI SQL standard, which is to always return the left most TRUE case. Line 179: // This WHEN is always TRUE, and any cases preceding it are constant > explain the implied 'else' case: that it's always false and therefore can b Done -- 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: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
