Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10858 )
Change subject: IMPALA-7027: Mulitple varchar cast fails with distinct ...................................................................... Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java File fe/src/main/java/org/apache/impala/analysis/StringLiteral.java: http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@63 PS1, Line 63: localEquals I know we don't currently have some unit tests for this, but I think we should. The unit tests can also provide some sort of documentation what the behavior should be. As far as the CR is concerned, we can limit the scope to only StringLiteral tests. http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@65 PS1, Line 65: (!(type_ == null ? that.type_ == null : type_ == that.type_)) > That wouldn't catch the case where type_==null and that.type_ != null, whic In that case, you can change it to if ((type != null || that.type_ != null) && type_ != that.type_) return false; Doing an inverse of a boolean statement that returns another boolean can be a bit difficult to read. -- To view, visit http://gerrit.cloudera.org:8080/10858 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2fa5890eaa89787645c7d3d2eef976f54a34e7c0 Gerrit-Change-Number: 10858 Gerrit-PatchSet: 2 Gerrit-Owner: Adam Holley <[email protected]> Gerrit-Reviewer: Adam Holley <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Comment-Date: Fri, 06 Jul 2018 17:11:33 +0000 Gerrit-HasComments: Yes
