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 <m...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-HasComments: Yes
