Mike Percy has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
......................................................................
Patch Set 3:
(3 comments)
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/paste to me, can we avoid that by extracting a helper method?
Also, it would be nice to do SetString(2, src_str) and then have src_str go out
of scope before continuing. That way we get ASAN to do the work for us.
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 in
existing tests and is less important to add coverage for.
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.
slices pointing to the same buffer). Personally I don't find that these types
of tests add a lot of value because they are so brittle. However if you want to
include this check then I'm alright with it.
Usually I would rather use something a little higher level to assert that the
contract provided by the interface is respected. That's easy to do with the
Copy() version and hard to do with the NoCopy() version, I think.
--
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
