Marcel Kornacker has posted comments on this change. Change subject: IMPALA-5137: Support Kudu UNIXTIME_MICROS as Impala TIMESTAMP ......................................................................
Patch Set 5: (5 comments) some more quick comments, haven't look at the logic in detail yet. http://gerrit.cloudera.org:8080/#/c/6526/5/be/src/exec/kudu-scanner.cc File be/src/exec/kudu-scanner.cc: Line 185: Tuple* kudu_tuple = const_cast<Tuple*>(reinterpret_cast<const Tuple*>( how come this expression changed so much? Line 191: // int64) have 8 bytes of padding following the timestamp. Because this padding is in l149 you're saying it's still a todo... http://gerrit.cloudera.org:8080/#/c/6526/5/be/src/exec/kudu-table-sink.cc File be/src/exec/kudu-table-sink.cc: PS5, Line 304: E also print value? http://gerrit.cloudera.org:8080/#/c/6526/5/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: Line 32: #include "gutil/walltime.h" i'm assuming you're including this because you're inlining some function bodies. please break those (=all inlined functions in this file) out into a timestamp-value.inl.h file and remove this include. sorry, but we need to start cleaning this up instead of piling on more includes in .h files, compilation times are really painful. Line 190: DCHECK(unix_time != NULL); switch to nullptr throughout this file -- 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: 5 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
