Attila Bukor 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 2: (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? Done http://gerrit.cloudera.org:8080/#/c/13928/1/src/kudu/common/partial_row.h@296 PS1, Line 296: max_column > It's not obvious what max_length is here. Could you clarify? Done http://gerrit.cloudera.org:8080/#/c/13928/1/src/kudu/common/partial_row.h@297 PS1, Line 297: used. This is : /// subject to change in the > See KuduScanner::SetRowFormatFlags for a more formal way to specify this. I'm not sure what you mean, I haven't found anything that would mean the behavior would be subject to change there. http://gerrit.cloudera.org:8080/#/c/13928/1/src/kudu/common/partial_row.h@307 PS1, Line 307: > Curious why you chose to add the Unsafe suffix? Are these fundamentally dif I wanted to indicate that this is not for 'normal' as it doesn't guarantee the behavior one might expect (no truncation). Should I remove it or do you have a better way to indicate this? 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? Well, it would mean checking one character only which shouldn't be too expensive, but it seems kind of arbitrary and weird to not check for actual length but still check whether the last character is a whitespace or not. Let me know what you think. -- 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: 2 Gerrit-Owner: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 26 Jul 2019 20:02:44 +0000 Gerrit-HasComments: Yes
