Alexey Serbin has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
......................................................................
Patch Set 2:
(3 comments)
> (3 comments)
>
> Can we add basic test coverage for SetString() and SetBinary()?
Yes, why not? Will do.
http://gerrit.cloudera.org:8080/#/c/3868/2/src/kudu/client/stubs.h
File src/kudu/client/stubs.h:
Line 66: // For deprecated functions or variables, generate a warning at usage
sites.
> If you want to bring in this helper as part of this patch, just mention it
OK, let me make a separate changelist bringing in this ATTRIBUTE_DEPRECATED
macro. I think it will be better that way.
As for presence of this macro in both stubs.h and port.h: as I understand,
port.h is for usage within the project, and stubs.h is distributed along with
our client C++ library and other files necessary for development of client-side
applications using Kudu C++ client API. So, that's why two separate files.
You can find that other macros which are used both throughout the project
internally and by the client C++ API utilize the same approach: e.g., check for
ATTRIBUTE_UNUSED.
http://gerrit.cloudera.org:8080/#/c/3868/2/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
Line 277: return SetSliceCopy<TypeTraits<BINARY> >(col_name, val);
> Can we just delegate these calls to SetBinaryCopy() so it's clear it's just
That's was my initial approach (you can see it in the earlier versions). Then
I realized there is some extra-call, and decided to switch to this version.
OK, I'll bring the older version back.
http://gerrit.cloudera.org:8080/#/c/3868/2/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
Line 92: // Sets the binary/binary value, copying 'val' data immediately.
Essentially,
> typo: binary/binary
Done
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-HasComments: Yes
