Alexey Serbin has posted comments on this change.

Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data

Patch Set 2:


 > Can we add basic test coverage for SetString() and SetBinary()?

Yes, why not?  Will do.
File src/kudu/client/stubs.h:

Line 66: // For deprecated functions or variables, generate a warning at usage 
> 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 
File src/kudu/common/

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.
File src/kudu/common/partial_row.h:

Line 92:   // Sets the binary/binary value, copying 'val' data immediately. 
> typo: binary/binary

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-HasComments: Yes

Reply via email to