Marcel Kornacker has posted comments on this change. Change subject: IMPALA-3856,IMPALA-3871: Fix BinaryPredicate normalization for Kudu ......................................................................
Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/3986/5/fe/src/main/java/com/cloudera/impala/analysis/BinaryPredicate.java File fe/src/main/java/com/cloudera/impala/analysis/BinaryPredicate.java: Line 118: * Normalizes a 'predicate' consisting of an uncast SlotRef and a constant Expr into what was wrong with the previous version of this function? given just the description in the function comment, it's not clear why this is disabled by casting. you should leave a comment in here, so the next person doesn't try to add the casting logic again. also, doesn't this break other things? i remember we specifically added the casting logic to address a problem. http://gerrit.cloudera.org:8080/#/c/3986/5/fe/src/main/java/com/cloudera/impala/analysis/Expr.java File fe/src/main/java/com/cloudera/impala/analysis/Expr.java: Line 1193: * For children of 'this' that are constant expressions capable of being expressed which constants cannot be expressed as literals? http://gerrit.cloudera.org:8080/#/c/3986/5/fe/src/main/java/com/cloudera/impala/analysis/LiteralExpr.java File fe/src/main/java/com/cloudera/impala/analysis/LiteralExpr.java: Line 55: public static LiteralExpr create(String value, Type type) throws AnalysisException { remove throw Line 154: * Returns null for types that cannot be expressed as literals, e.g. TIMESTAMP. you mean for which don't have a LiteralExpr subclass? 'cannot be expressed' sounds like a sql limitation. -- To view, visit http://gerrit.cloudera.org:8080/3986 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iae7612433a2e27f8887abe6624f9ee0f4867b934 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
