Jim Apple has posted comments on this change. Change subject: IMPALA-1903: Add support for partitioning by TIMESTAMP ......................................................................
Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/1621/3/be/src/exprs/expr.cc File be/src/exprs/expr.cc: Line 209: else { : *expr = pool->Add(new NullLiteral(texpr_node)); : } > It is debatable whether turning invalid values to NULLs is the correct thin It might be possible to treat invalid timestamps read from directory structures as NULL and timestamps read from Impala user input as reasons to reject a query, but it looks to me like these things are all tied up with each other now, so it may take some detangling. What is your opinion on how Impala should behave on timestamp creation via "INSERT INTO table PARTITION(cast(col as timestamp)) SELECT * from table2" where a timestamp is invalid? http://gerrit.cloudera.org:8080/#/c/1621/3/fe/src/main/java/com/cloudera/impala/analysis/TimestampLiteral.java File fe/src/main/java/com/cloudera/impala/analysis/TimestampLiteral.java: Line 84: public int compareTo(LiteralExpr o) { > I am not so sure about that. If this Literal implements compareTo, it sends Unfortunately, I think we might be stuck here - TimestampLiteral has to extend LiteralExpr, and LiteralExpr already implements Comparable<LiteralExpr>. I could make it an error at run time. -- 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: 3 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
