Todd Lipcon has posted comments on this change. Change subject: KUDU-1307 [master] support tables with non-covering partition-key ranges ......................................................................
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 lines below Line 323: } can you add a case here like I mentioned elsewhere in this review? where a split row falls in a gap? another edge case might be a split row falling exactly on the upper or lower bound http://gerrit.cloudera.org:8080/#/c/2806/8/src/kudu/common/partition.cc File src/kudu/common/partition.cc: Line 202: int bool Line 218: return Status::OK(); should you key->clear() here? given the name is 'Encode' and not 'AppendEncoded' or something, I would expect it to replace. Or, add a DCHECK(key->empty()) at the top Line 226: for (const KuduPartialRow& row : split_rows) { same, should clarify whether this is appending to 'splits' or replacing the current value. If the latter, should clear or assert that it's empty Line 239: return Status::InvalidArgument("Duplicate split row"); can you stringify unique_end->ToString() here to get the duplicate row into the error? Line 253: for (const tuple<KuduPartialRow, KuduPartialRow>& bound : range_bounds) { maybe auto Line 260: return Status::InvalidArgument("Range bound has lower bound above the upper bound"); stringify the bad pair 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 variable arguments to the constructor? (same thing elsewhere) Line 272: return Status::InvalidArgument("Overlapping range bounds"); again would be nice to stringify (same comment in a few bad statuses below) Line 297: return Status::InvalidArgument("Split out of bounds"); should be careful to document that, in the case of error, this function still mutates its arguments. I think that strangeness might not be worth the marginal perf gain for this not-hot-path, and instead you could just copy bounds_temp = *bounds; 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. Line 289: // must be sorted. what happens if there's a split point that doesn't fall within a 'bound' pair? eg: splits: {5, 10} bounds: {<1, 3>, <15,20>} Is that an error? should specify in the docs Also, is it legal to not specify any bounds? i.e is the '<min,max>' implicit in that case? 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 quite a bit. 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 ('.first' and '.second' vs the weird-looking std::get<0>(...) ) Line 434: { // Split row out of bounds (below). mentioned elsewhere, would be good to test splits equal to the bounds (could either test here or in the partition unit test) Line 468: "Invalid argument: Range bound has lower bound above the upper bound"); perhaps the error should indicate "equal to or above" 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 compatibility of this patch? if we send the new stuff to the old master, will you get an error or silently wrong results? If the latter maybe we need to add a feature flag? Also, can you refer to somewhere here that will explain what exactly the 'bounds' mean? or explain it here? -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
