Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-1903: Add support for partitioning by TIMESTAMP ......................................................................
Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/1621/6/be/src/runtime/timestamp-parse-util.h File be/src/runtime/timestamp-parse-util.h: Line 359: Parse Sorry, I know I asked you to change the name, but I think something like IsValidTimestamp() or something along those lines seems more appropriate. What do you think? http://gerrit.cloudera.org:8080/#/c/1621/6/fe/src/main/java/com/cloudera/impala/analysis/TimestampLiteral.java File fe/src/main/java/com/cloudera/impala/analysis/TimestampLiteral.java: Line 29: public class TimestampLiteral I think it deserves a class comment. Line 47: public String toSqlImpl() { : return "'" + value_ + "'"; : } single line Line 59: public String getStringValue() { : return value_; : } single line http://gerrit.cloudera.org:8080/#/c/1621/6/fe/src/main/java/com/cloudera/impala/planner/HdfsPartitionPruner.java File fe/src/main/java/com/cloudera/impala/planner/HdfsPartitionPruner.java: Line 211: !(child.isLiteral()) || child.getType().isTimestamp() I don't think this condition is correct. For example, "a IN (timestamp_col)" expr, where a is a partition column cannot be evaluated in the FE, but that condition will return false. We should also have a test that covers that case. -- To view, visit http://gerrit.cloudera.org:8080/1621 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icad7dcdc1b199cce9483dc414072bbe24efd625c Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-HasComments: Yes
