Dan Burkert has posted comments on this change. Change subject: KUDU-1307 [master] support tables with non-covering partition-key ranges ......................................................................
Patch Set 6: (11 comments) http://gerrit.cloudera.org:8080/#/c/2806/6/src/kudu/common/partition-test.cc File src/kudu/common/partition-test.cc: Line 65: void CheckCreateRangePartitions(vector<tuple<optional<string>, optional<string>>> raw_bounds, : vector<string> raw_splits, : vector<tuple<string, string>> expected_partition_ranges) { > Pass all of these by cref? Done http://gerrit.cloudera.org:8080/#/c/2806/6/src/kudu/common/partition.cc File src/kudu/common/partition.cc: Line 202: key->clear(); > In every call to EncodeRangeKey, 'key' is already an empty string. So perha Done Line 218: if (column_count == 0) { > Since you only care if column_count is zero or non-zero, perhaps make it a Done Line 227: splits->clear(); > This is already empty before the one call to EncodeRangeSplits. Perhaps we Done Line 250: range_partitions->clear(); > Already empty, can we omit? Done Line 275: return Status::InvalidArgument("overlapping range bounds"); > Nit: capital letter for the beginning of an error message. Elsewhere in the Done Line 282: Status PartitionSchema::SplitRangeBounds(vector<std::string> splits, > Nit: don't need std:: prefix here The strings contained in the vector are being modified via a move on line 307. http://gerrit.cloudera.org:8080/#/c/2806/6/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 749: if (op.type == RowOperationsPB::SPLIT_ROW) { > Nit: might be more clear as a switch statement: Done Line 753: if (i >= ops.size() || ops[i].type != RowOperationsPB::RANGE_UPPER_BOUND) { > Why must the very next row operation be the upper bound? Is that documented The bounds need to have an upper and lower bound. This is documented in wire_protocol.proto. Line 761: Substitute("illegal row operation type in create table request: $0", op.type)); > Nit: begin the error message with a capital letter. On L754 too. Done http://gerrit.cloudera.org:8080/#/c/2806/6/src/kudu/master/master.proto File src/kudu/master/master.proto: Line 343: optional RowOperationsPB split_rows_range_bounds = 6; > Renaming the field doesn't affect backwards compatibility, does it? backwards compatibility is unaffected. -- 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: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
