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, 08 May 2017 22:08:09 GMT
Matthew Jacobs has posted comments on this change.

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


Patch Set 6:

(7 comments)

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

Line 150:   row_format_flags |= kudu::client::KuduScanner::PAD_UNIXTIME_MICROS_TO_16_BYTES;
> why start with no_flags instead of setting this directly in l149?
Done


Line 201:       if (slot->type().type != TYPE_TIMESTAMP) continue;
> pull out timestamp slots into a separate vector (which you set up in prepar
Done


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

Line 111: Status WriteKuduRowValue(kudu::KuduPartialRow* row, int col, PrimitiveType type,
> fix signature (output params go at end).
Done.

I think the order may have been such that copy_strings can have a default parameter. I moved
'copy_strings' to a template variable though to avoid branching when calling this. That also
means this fn is moved to the header, feel free to push back if you think that warrants more
build-time impact than it is worth.


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

Line 19: #include "runtime/timestamp-value.inline.h"
> remove this, unless you're calling these functions here (which doesn't appe
Done


Line 98:     if (UNLIKELY(nullptr == localtime_r(&utc, &temp))) {
> fix order
Done


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

Line 25
> are there other includes you can remove?
Looks like I can remove another boost header. I think I still need gregorian and local_time
for types used in function definitions in this file. compiler_config appears to affect compilation
options [1], and seems risky to touch without understanding it more.

1: http://www.boost.org/doc/libs/1_64_0/boost/date_time/compiler_config.hpp


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

Line 34: bool TimestampValue::UtcToUnixTime(time_t* unix_time) const {
> you need to prefix all of these with inline.
Done


-- 
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: 6
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: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message