impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5137: Support Kudu UNIXTIME MICROS as Impala TIMESTAMP
Date Sat, 06 May 2017 17:47:40 GMT
Marcel Kornacker has posted comments on this change.

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


Patch Set 6:

(9 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?


Line 201:       if (slot->type().type != TYPE_TIMESTAMP) continue;
pull out timestamp slots into a separate vector (which you set up in prepare), no need to
check every column on every row whether it's a timestamp.


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).

also, is "row value" a kudu term (that means a column value)? to me this sounds like it's
writing a whole row, which isn't the case.


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 appear to be the case).
by including this you avoided linker errors, which would have forced you to include the .inline.h
in the referencing .cc files.


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


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

Line 193:   /// Interpret 'this' as a timestamp in UTC and convert to unix time in microseconds.
> I'd prefer to keep this consistent with the ToUnixTime() code (see l224), i
i don't know when that's going to happen. at least leave a todo in the implementation to revisit.


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?


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.

what you have right now removes the inlining and potentially degrades performance. (you need
to include the .inline.h file in every .cc file that includes the .h file right now and calls
the inlined functions.)


http://gerrit.cloudera.org:8080/#/c/6526/5/testdata/workloads/functional-query/queries/QueryTest/kudu_insert.test
File testdata/workloads/functional-query/queries/QueryTest/kudu_insert.test:

Line 6:    ts timestamp)
> 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