impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
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:

(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 <mj@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message