Adar Dembo 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? 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 perhaps we can omit this? Line 218: if (column_count == 0) { Since you only care if column_count is zero or non-zero, perhaps make it a boolean with a more appropriate name? Line 227: splits->clear(); This is already empty before the one call to EncodeRangeSplits. Perhaps we can omit? Line 250: range_partitions->clear(); Already empty, can we omit? Line 275: return Status::InvalidArgument("overlapping range bounds"); Nit: capital letter for the beginning of an error message. Elsewhere in the new code too. Line 282: Status PartitionSchema::SplitRangeBounds(vector<std::string> splits, Nit: don't need std:: prefix here Also, why aren't we passing splits by cref? We're not actually modifying the vector, are we? 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: switch (op.type) { case SPLIT_ROW: ... case RANGE_LOWER_BOUND: ... default: return "Illegal row operation type" } 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 somewhere in the protocol? 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. 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? -- 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: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
