Lars Volker has posted comments on this change. Change subject: IMPALA-5137: Support Kudu UNIXTIME_MICROS as Impala TIMESTAMP ......................................................................
Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/6526/3/be/src/exec/kudu-table-sink.cc File be/src/exec/kudu-table-sink.cc: Line 302: DCHECK(success) << "Invalid TimestampValue"; This DCHECK effectively tests tv->HasDateAndTime(). Is there any way this could fail? In a release build we would read the uninitialized ts_micros in the line below. Would it be an option to use RETURN_IF_FALSE here? It could also be inlined then. http://gerrit.cloudera.org:8080/#/c/6526/3/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java File fe/src/main/java/org/apache/impala/planner/KuduScanNode.java: Line 125: String colName = desc.getColumn().getName(); Nit: Now that I look at this you could also inline it. But I'm don't feel strongly about it at all, so feel free to keep it like it is if you prefer it. http://gerrit.cloudera.org:8080/#/c/6526/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java File fe/src/main/java/org/apache/impala/util/KuduUtil.java: Line 186: default: Where did this go? -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
