Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5137: Support Kudu UNIXTIME_MICROS as Impala TIMESTAMP ......................................................................
Patch Set 2: (3 comments) Thanks for the input Lars, sorry for the delay. I've been waiting to do the read path as well. I have a patch I'll post soon. http://gerrit.cloudera.org:8080/#/c/6526/3/be/src/exec/kudu-table-sink.cc File be/src/exec/kudu-table-sink.cc: Line 302: KUDU_RETURN_IF_ERROR(write->mutable_row()->SetUnixTimeMicros(col, ts_micros), > This DCHECK effectively tests tv->HasDateAndTime(). Is there any way this c HasDateAndTime checks that both the boost date or boost time are not 'special' values (ie. +/- inf, not_a_date_time). If these are 'special' the slot *should be* null. AFAIK there is no way to represent +/- inf or 'not_a_date_time' as an Impala TIMESTAMP -- they'd be NULL. timestamp-value.h doesn't really explain, this is all based on me reading the code, so if you or others think otherwise please let me know :) That said, I guess this should be defensive and also RETURN_IF_FALSE for release builds, so I'll continue to DCHECK but will also RETURN_IF_FALSE for avoiding unexpected behavior in release builds. http://gerrit.cloudera.org:8080/#/c/6526/3/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java File fe/src/main/java/org/apache/impala/planner/KuduScanNode.java: Line 125: Type colType = desc.getColumn().getType(); > Nit: Now that I look at this you could also inline it. But I'm don't feel s this goes away anyway :) http://gerrit.cloudera.org:8080/#/c/6526/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java File fe/src/main/java/org/apache/impala/util/KuduUtil.java: Line 186: case UNIXTIME_MICROS: > Where did this go? I'm going to do all the FE-related changes in a future patch. (And this is going to be a bit more tricky than it looks because the literal needs to be converted to a unixtime micros from a Timestamp.) -- To view, visit http://gerrit.cloudera.org:8080/6526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iae6ccfffb79118a9036fb2227dba3a55356c896d Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-HasComments: Yes
