Dan Burkert has posted comments on this change. Change subject: KUDU-1307 [master] support tables with range partition bounds ......................................................................
Patch Set 8: (18 comments) http://gerrit.cloudera.org:8080/#/c/2806/8/src/kudu/common/partition-test.cc File src/kudu/common/partition-test.cc: Line 80: for (const tuple<optional<string>, optional<string>>& tuple : raw_bounds) { > i think const auto& is fine here assuming you keep the types on the two lin Done Line 323: } > can you add a case here like I mentioned elsewhere in this review? where a yep, both cases are covered in master-test http://gerrit.cloudera.org:8080/#/c/2806/8/src/kudu/common/partition.cc File src/kudu/common/partition.cc: Line 202: int > bool Done Line 218: return Status::OK(); > should you key->clear() here? given the name is 'Encode' and not 'AppendEnc Done Line 226: for (const KuduPartialRow& row : split_rows) { > same, should clarify whether this is appending to 'splits' or replacing the Done Line 239: return Status::InvalidArgument("Duplicate split row"); > can you stringify unique_end->ToString() here to get the duplicate row into Done Line 253: for (const tuple<KuduPartialRow, KuduPartialRow>& bound : range_bounds) { > maybe auto Done Line 260: return Status::InvalidArgument("Range bound has lower bound above the upper bound"); > stringify the bad pair Done Line 262: range_partitions->emplace_back(make_tuple(std::move(lower), std::move(upper))); > why do you need make_tuple here, given emplace_back already forwards its va You are right that it's unnecessary. Fixed. Line 272: return Status::InvalidArgument("Overlapping range bounds"); > again would be nice to stringify (same comment in a few bad statuses below) Done Line 297: return Status::InvalidArgument("Split out of bounds"); > should be careful to document that, in the case of error, this function sti Done http://gerrit.cloudera.org:8080/#/c/2806/8/src/kudu/common/partition.h File src/kudu/common/partition.h: Line 151: const std::vector<std::tuple<KuduPartialRow, > add method docs about how these interact with the split rows. Done Line 289: // must be sorted. > what happens if there's a split point that doesn't fall within a 'bound' pa Done http://gerrit.cloudera.org:8080/#/c/2806/8/src/kudu/common/wire_protocol.proto File src/kudu/common/wire_protocol.proto: Line 141: // A set of operations (INSERT, UPDATE, or DELETE) to apply to a table. > this is probably due for an update now that we're overloading this code qui Done http://gerrit.cloudera.org:8080/#/c/2806/8/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: Line 86: const vector<tuple<KuduPartialRow, KuduPartialRow>>& bounds); > curious why you choose tuple instead of pair? pair seems more natural to me Done Line 434: { // Split row out of bounds (below). > mentioned elsewhere, would be good to test splits equal to the bounds (coul Done Line 468: "Invalid argument: Range bound has lower bound above the upper bound"); > perhaps the error should indicate "equal to or above" Done http://gerrit.cloudera.org:8080/#/c/2806/8/src/kudu/master/master.proto File src/kudu/master/master.proto: Line 342: // Holds either the split rows or the range bounds (or both) of the table. > can you add something to the commit message about the wire protocol compati done, and I've added further color to the RowOperationsPB doc. -- To view, visit http://gerrit.cloudera.org:8080/2806 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I04a7ed3e85c993fdbcb313a419f15031103736ce Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
