Matthew Jacobs 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? For Kudu predicates, at least, we can't have the slotref wrapped by a cast. So this fn now returns the very specific kind of predicate described here if possible. This shouldn't break anything else because its only caller is the KuduScanNode. Would you prefer if I change the name of the fn to normalizeNoCastSlotRefComparison() (or similar)? 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? DATE/DATETIME/TIMESTAMP don't have LiteralExpr subclasses. I'll change. 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 analyze(), uncheckedCastTo(), and BoolLiteral()/NumericLiteral() all throw AnalysisEx. 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' Yes, I'll change the comment -- 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
