Alexey Serbin has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
......................................................................
Patch Set 3:
(3 comments)
Will post the updated version soon.
http://gerrit.cloudera.org:8080/#/c/3868/3/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
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>(src_str.data()),
> 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 slice.data() == str.data().
Is there a mistake in the chain above?
--
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: 3
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
