Jim Apple 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 Is I'm ok with IsValidTimestamp. 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. Done Line 47: public String toSqlImpl() { : return "'" + value_ + "'"; : } > single line Done Line 59: public String getStringValue() { : return value_; : } > single line Done 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) I added tests for "a IN (b)" for a non-partition string col | b partition timestamp col a partition string col | b non-partition timestamp col a non-partition timestamp col | b partition timestamp col a non-partition timestamp col | b partition string col a partition timestamp col | b non-partition timestamp col a partition timestamp col | b non-partition string col Does that cover what you were expecting? -- 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
