Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15042 )
Change subject: cfile: change BlockBuilder API to yield a vector of Slices ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/binary_dict_block.cc File src/kudu/cfile/binary_dict_block.cc: http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/binary_dict_block.cc@85 PS1, Line 85: vector<Slice> data_slices; : data_builder_->Finish(ordinal_pos > isn't that equivalent from a performnace perspective? in either case, you'r yep, it seems the same since and it's called just once here. at least, maybe add appropriate data_slices.reserve()? http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/cfile_writer.cc File src/kudu/cfile/cfile_writer.cc: http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/cfile_writer.cc@399 PS1, Line 399: ck(null_hea > I think given Slice is a simple value type there isn't any benefit to movin yup, indeed http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/encoding-test.cc File src/kudu/cfile/encoding-test.cc: http://gerrit.cloudera.org:8080/#/c/15042/1/src/kudu/cfile/encoding-test.cc@127 PS1, Line 127: contiguous_buf_ > nope, because the Slice doesn't own the pointed-to memory, so it has to out Ah, indeed. Does it mean the callers of this method should expect some strange results if calling this method twice in a row if they keep the returned results at the upper level? Maybe, it make sense to protect against that replacing contiguous_buf_.clear() with CHECK(contiguous_buf_.empty()) ? -- To view, visit http://gerrit.cloudera.org:8080/15042 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifc7a5f148a4a43cedac2428f4c1a18d0f93a10db Gerrit-Change-Number: 15042 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Thu, 16 Jan 2020 18:28:46 +0000 Gerrit-HasComments: Yes
