Alexey Serbin has posted comments on this change.

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

Patch Set 3:


Will post the updated version soon.
File src/kudu/common/

Line 189: TEST_F(PartialRowTest, TestSetBinaryAndString) {
> TestSetBinaryAndString and TestSetBinaryAndStringCopy seem like mostly copy
Yes, it's a good idea to have a helper method for those.  Will do.

As for the ASAN-related thing, I do not see how to get the desired behavior in 
a manageable way.  As I see it, there are two questions here:

1. It's guaranteed that the use-after-delete will be visible at non-ASAN build, 
but we want this test to be reliably working under any type of build, right?

2. Even if we are targeting this for ASAN-builds only, is it guaranteed the 
deleted memory will be filled with some predictable pattern which we can rely 
on for current and future versions of the ASAN library?

Line 262:   ASSERT_OK(row.SetStringNoCopy(2, src_str));
> It's up to you but personally I think NoCopy already has a lot of coverage 
SetXxxNoCopy() are the newly introduced methods, so I think it's nice to have 
at least some basic coverage for them.

Line 276:   EXPECT_EQ(reinterpret_cast<uintptr_t>(,
> I think this just asserts that the implementation works a certain way (i.e.
I thought this part was derived from the interface constraints for the Slice 
and PartialRow classes:

1. The PartialRow::Get{Binary,String}() methods return the stored data, and 
they do not copy the data, as reflected in corresponding comments for those 
methods in the partial_row.h file.

2. By definition, the Slice class is a wrapper around the data, and it's data() 
method should return the pointer that corresponds to the source string's data 
in this case.  When we create a Slice object using Slice(const string& s) 
constructor, the result object should refer to the data of the original string.

3. The PartialRow::Set{String,Binary}NoCopy() set the value for the specified 
column, making no copies of the passed data.

Given 1, 2, and 3, we should expect ==

Is there a mistake in the chain above?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 3
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