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