Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/13928 )
Change subject: KUDU-1938 Add non-copy VARCHAR setters pt 3 ...................................................................... Patch Set 19: (1 comment) http://gerrit.cloudera.org:8080/#/c/13928/19/src/kudu/common/partial_row.h File src/kudu/common/partial_row.h: http://gerrit.cloudera.org:8080/#/c/13928/19/src/kudu/common/partial_row.h@306 PS19, Line 306: Status SetVarcharNoCopyUnsafe(const Slice& col_name, const Slice& val) WARN_UNUSED_RESULT; > Sorry I missed that. Ideally no-one would use this to actually persist data, Impala only uses it to decide which bucket a given primary key falls to and by then the data is pre-truncated. The check is there only to make sure no one uses this method to bypass the length limitation of the VARCHAR to persist huge amounts of data. Even assuming only ASCII characters would be used and with the fast path that is coming in https://gerrit.cloudera.org/c/14353 we need to load the whole value and perform bitwise operations on it chunk by chunk. This seems to me like a bigger performance hit than a simple memcpy which we still optimized out in SetStringNoCopy and SetBinaryNoCopy. The potential bugs are why I adde the "Unsafe" suffix, hopefully this would deter users from using this version. Todd also mentioned doing a proper check here instead in https://gerrit.cloudera.org/c/13928/3//COMMIT_MSG#17, but as I said to him I'm happy to change it if you think we should suffer the performance hit rather than allow abusing the API that would potentially lead to bugs. -- 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: 19 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 03 Oct 2019 07:11:51 +0000 Gerrit-HasComments: Yes
