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 <m...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-HasComments: Yes
