Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13928 )
Change subject: KUDU-1938 Add non-copy setters to partial row pt 3 ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/13928/1/src/kudu/common/partial_row.h File src/kudu/common/partial_row.h: http://gerrit.cloudera.org:8080/#/c/13928/1/src/kudu/common/partial_row.h@274 PS1, Line 274: /// @note The specified data must remain valid until the corresponding : /// RPC calls are completed to be able to access error buffers, : /// if any errors happened (the errors can be fetched using the : /// KuduSession::GetPendingErrors() method). Should this @note be copied in? http://gerrit.cloudera.org:8080/#/c/13928/1/src/kudu/common/partial_row.h@296 PS1, Line 296: max_length It's not obvious what max_length is here. Could you clarify? http://gerrit.cloudera.org:8080/#/c/13928/1/src/kudu/common/partial_row.h@297 PS1, Line 297: This is subject : /// to change in the future. See KuduScanner::SetRowFormatFlags for a more formal way to specify this. http://gerrit.cloudera.org:8080/#/c/13928/1/src/kudu/common/partial_row.h@307 PS1, Line 307: Unsafe Curious why you chose to add the Unsafe suffix? Are these fundamentally different than the existing NoCopy functions? http://gerrit.cloudera.org:8080/#/c/13928/1/src/kudu/common/partial_row.cc File src/kudu/common/partial_row.cc: http://gerrit.cloudera.org:8080/#/c/13928/1/src/kudu/common/partial_row.cc@423 PS1, Line 423: Status KuduPartialRow::SetCharNoCopyUnsafe(int col_idx, const Slice& val) { Should we validate that there's no trailing whitespace in the CHAR method? Or is that too expensive? -- To view, visit http://gerrit.cloudera.org:8080/13928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f2aba098d649eb94e0314f6606cc33600e8d766 Gerrit-Change-Number: 13928 Gerrit-PatchSet: 1 Gerrit-Owner: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 26 Jul 2019 18:17:39 +0000 Gerrit-HasComments: Yes
