impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3856,IMPALA-3871: Fix BinaryPredicate normalization for Kudu
Date Fri, 19 Aug 2016 15:49:13 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3856,IMPALA-3871: Fix BinaryPredicate normalization for Kudu

Patch Set 5:

File fe/src/main/java/com/cloudera/impala/analysis/

Line 118:    * Normalizes a 'predicate' consisting of an uncast SlotRef and a constant Expr
> 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)?
File fe/src/main/java/com/cloudera/impala/analysis/

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.
File fe/src/main/java/com/cloudera/impala/analysis/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Iae7612433a2e27f8887abe6624f9ee0f4867b934
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-HasComments: Yes

View raw message