Dimitris Tsirogiannis 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 might be possible to treat invalid timestamps read from directory struct Ideally, I would like to treat it the same way we treat the following scenario: table foo (..) partitioned by (x int) table bla (.., col string) and values "1", "2", "invalid" in col insert into table foo partition (cast(col as int)) select * from bla; I hope we fail in a case like this. If not, we just discovered yet another inconsistency.... 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) { > Unfortunately, I think we might be stuck here - TimestampLiteral has to ext Not really. The default implementation of compareTo in LiteralExpr is well specified and simply enforces an order wrt NULLs (all NULLs before or after valid values). I think this is ok. Overriding it on the other hand here indicates that you can order TimestampValues in some other meaningful way. I am just saying that since we don't parse values in the FE, the order imposed by this implementation of compareTo() is not "meaningful" and at the same time it may trick users of this class to thinking they can use it in places where it shouldn't be used. -- 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
