Adar Dembo has posted comments on this change. Change subject: Reorganize range partition client API ......................................................................
Patch Set 3: (14 comments) http://gerrit.cloudera.org:8080/#/c/3882/3/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: Line 1288: STLDeleteElements(&errors); Use an ElementDeleter instead, and declare it just after 'errors'. http://gerrit.cloudera.org:8080/#/c/3882/3/src/kudu/client/client.h File src/kudu/client/client.h: Line 541: KuduTableCreator& add_range_partition(KuduPartialRow* lower_bound, Should add @param entries for the bound types. Line 808: KuduTableAlterer* AddRangePartition( Should add @param entries for the bound types. Line 835: KuduTableAlterer* DropRangePartition( Should add @param entries for the bound types. http://gerrit.cloudera.org:8080/#/c/3882/3/src/kudu/common/partition-test.cc File src/kudu/common/partition-test.cc: Line 657: auto check = [&] (const vector<boost::optional<int8_t>>& test, bool lower_bound) { Could you precondition up front that 'test' has the expected number of elements? Since you're using operator[] to dereference, you'll just get undefined behavior if you accidentally provide fewer elements than you expected. Below too. http://gerrit.cloudera.org:8080/#/c/3882/3/src/kudu/common/partition.cc File src/kudu/common/partition.cc: Line 921: switch (row->schema()->column(idx).type_info()->physical_type()) { What about STRING and TIMESTAMP? The other types not present here (like BOOL, FLOAT, and DOUBLE) aren't supported in range partitioning, right? Line 1042: } The behavior of "increment does nothing for a maxed out partition key" should be documented in the function declaration. PS3, Line 1048: // To transform a lower bound range partition key from exclusive to inclusive, : // the key mut be incremented. To increment the key, start with the least : // significant column in the key (furthest right), and increment it. If the : // increment fails because the value is already the maximum, move on to the : // next least significant column and attempt to increment it (and so on). When : // incrementing, an unset cell is equivalent to a cell set to the minimum : // value for its column (e.g. an unset Int8 column is incremented to -127 : // (-2^7 + 1)). Finally, all columns less significant than the incremented : // column are unset (which means they are treated as the minimum value for : // that column). If all columns in the key are the maximum and can not be : // incremented, then the operation fails. : // : // A few examples, given a range partition of three Int8 columns. Underscore : // signifies unset: : // : // (1, 2, 3) -> (1, 2, 4) : // (1, 2, 127) -> (1, 3, _) : // (1, 127, 3) -> (1, 127, 4) : // (1, _, 3) -> (1, _, 4) : // (_, _, _) -> (_, _, 1) : // (1, 127, 127) -> (2, _, _) : // (127, 127, 127) -> fail! I think this is better placed in IncrementRangePartitionKey(). http://gerrit.cloudera.org:8080/#/c/3882/3/src/kudu/common/partition.h File src/kudu/common/partition.h: PS3, Line 194: // Transforms an inclusive lower bound range partition key into an inclusive : // lower bound range partition key. So...the transformation does nothing? :) I think you meant 'exclusive' the first time you wrote 'inclusive'. PS3, Line 198: // Transforms an exclusive upper bound range partition key into an in : // exclusive upper bound range partition key. And here I think you meant 'inclusive' first. And there's an extra 'in'. http://gerrit.cloudera.org:8080/#/c/3882/3/src/kudu/common/wire_protocol.proto File src/kudu/common/wire_protocol.proto: PS3, Line 155: // Used when specifying an inclusive lower bound range on table creation. : // Should be followed by the associated upper bound. If all values are : // missing, then signifies unbounded. : RANGE_LOWER_BOUND = 6; : // Used when specifying an exclusive upper bound range on table creation. : // Should be preceded by the associated lower bound. If all values are : // missing, then signifies unbounded. : RANGE_UPPER_BOUND = 7; Can we add EXCLUSIVE/INCLUSIVE to these two? Or have they already been released? Line 164: // Should be followed by the associated upper bound. Nit: "If all values are missing..." http://gerrit.cloudera.org:8080/#/c/3882/3/src/kudu/integration-tests/alter_table-randomized-test.cc File src/kudu/integration-tests/alter_table-randomized-test.cc: Line 461: table_alterer->AddRangePartition(lower_bound.release(), upper_bound.release()); Can we get some coverage here and below of the different exclusive/inclusive bound types? http://gerrit.cloudera.org:8080/#/c/3882/3/src/kudu/integration-tests/alter_table-test.cc File src/kudu/integration-tests/alter_table-test.cc: Line 1055: TEST_F(AlterTableTest, TestAlterRangePartitioning) { Testing the new bound types here would be good too. -- To view, visit http://gerrit.cloudera.org:8080/3882 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic731f0abcbb81cd4238cc960b150bf8cd1c1f0ea Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
