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 Mon, 03 Apr 2017 19:59:36 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5137: Support Kudu UNIXTIME_MICROS as Impala TIMESTAMP
......................................................................


Patch Set 1:

(7 comments)

Thanks for taking a look, Lars. This was meant to be a WIP because I (a) wanted to get some
input on the TimestampValue interface changes and (b) am waiting for the Kudu folks to provide
a new API for reading unixtime_micros in an Impala friendly format. Sorry I didn't make that
clear.

http://gerrit.cloudera.org:8080/#/c/6526/1/be/src/exec/kudu-util.cc
File be/src/exec/kudu-util.cc:

Line 29: #include "runtime/timestamp-value.h"
> Why is this needed?
Done


http://gerrit.cloudera.org:8080/#/c/6526/1/be/src/runtime/timestamp-test.cc
File be/src/runtime/timestamp-test.cc:

PS1, Line 596: 17987443200
> These values all look special but it's hard to tell why they are like that.
sure, I cleaned up the min/max dates in unixtimes that I was touching here


http://gerrit.cloudera.org:8080/#/c/6526/1/be/src/runtime/timestamp-value.h
File be/src/runtime/timestamp-value.h:

PS1, Line 201: reinterpret_cast<int64_t>
> Will this work if time_t is 32bit? I think static_cast<>() would be the pre
I think this should fail to compile if time_t is 32 bit. I agree this should be a static_cast.


http://gerrit.cloudera.org:8080/#/c/6526/1/fe/src/main/java/org/apache/impala/util/KuduUtil.java
File fe/src/main/java/org/apache/impala/util/KuduUtil.java:

Line 151:       //  TODO: Can't support timestamps in the key until FE can convert to unixtime
> Will you address this in your change? If not, can you create a JIRA?
I'm not sure yet, I certainly won't leave commented out code in here. This was meant to be
a preview :)


http://gerrit.cloudera.org:8080/#/c/6526/1/tests/query_test/test_kudu.py
File tests/query_test/test_kudu.py:

Line 112:         (cast("1970-01-01 00:00:00" as timestamp)),
> nit: Make all keywords the same case (upper or lower).
Done


Line 113:         (cast("1970-01-01 00:00:00" as timestamp) + interval 1 microsecond)
> Can you add a timestamp before the epoch (so it'd be negative), and one in 
sure, will add them along with more tests, especially when the read path can be implemented.
This is mostly here as a holdover as I wait for the Kudu work to add read support. I'd like
to have extensive tests in a .test file.


PS1, Line 125: simle
> nit simple
fixing simple

the issue with the tzinfo is that it's defined as an abstract class, so I'd have to provide
an implementation as well which I didn't think was worth it


-- 
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message