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: (3 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@297 PS1, Line 297: used. This is : /// subject to change in the > The doxygen comments start with: sorry, I missed that one. Should I just change the name to Advanced/Unstable API and push down the current title to the description? Should I leave the current description as well as an explanation? http://gerrit.cloudera.org:8080/#/c/13928/1/src/kudu/common/partial_row.h@307 PS1, Line 307: > When I read "Unsafe" I tend to think of memory accesses, or of APIs that pl These methods shouldn't be used by clients other than Impala so that part of the 'unsafe' contract is there. It doesn't cause memory though but clients could abuse it and add entries with 4 * max_column_length characters if only ASCII or 'traditional' code pages are use which is why I thought it should be unsafe. 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) { > Hmm, I guess I can see that. Could you doc this limitation in the CHAR doxy Done -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 26 Jul 2019 22:43:37 +0000 Gerrit-HasComments: Yes
