impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5137: Support Kudu UNIXTIME MICROS as Impala TIMESTAMP
Date Fri, 21 Apr 2017 00:42:49 GMT
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 <mj@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <davidralves@gmail.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message