[kudu-CR] Reorganize range partition client API

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: Reorganize range partition client API
..


Reorganize range partition client API

This commit redesigns the client APIs dealing with adding and dropping range
partitions. The goal is to make them more consistent and easier to understand.
The biggest public facing change is that all methods which deal with range
partitions are now named that, and the 'bounds' nomenclature has mostly been
dropped. All of these methods are new since 0.9.1, so this shouldn't be a
breaking change (wire compatibility is not broken).

Additionally, range partitions can now be created, added, and dropped with
exclusive lower bounds and inclusive upper bounds. Originally I thought this
would be a pretty minor feature, but I think it's going to end up being useful
when creating higher level SQL bindings.

Finally, the error messages when altering tables have been significantly
improved with new pretty printing of the offending range partition. Before this
it would print the serialized range partition keys, which are pretty much
impossible to figure out.

An equivalent cleanup for the Java client will be in a follow up commit.

Change-Id: Ic731f0abcbb81cd4238cc960b150bf8cd1c1f0ea
Reviewed-on: http://gerrit.cloudera.org:8080/3882
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_alterer-internal.cc
M src/kudu/client/table_alterer-internal.h
M src/kudu/client/table_creator-internal.h
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/wire_protocol.proto
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/flex_partitioning-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tserver/tablet_server-test.cc
20 files changed, 838 insertions(+), 190 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/3882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ic731f0abcbb81cd4238cc960b150bf8cd1c1f0ea
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Reorganize range partition client API

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Reorganize range partition client API
..


Patch Set 5: Code-Review+2

-- 
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Reorganize range partition client API

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Reorganize range partition client API
..


Patch Set 5:

Build Started http://104.196.14.100/job/kudu-gerrit/2843/

-- 
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Reorganize range partition client API

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Reorganize range partition client API
..


Patch Set 4: Code-Review+1

(2 comments)

basically just looked at the API, rather than the impl, since it looks like 
Adar's already looking through the impl. API looks good.

http://gerrit.cloudera.org:8080/#/c/3882/4/src/kudu/client/client.h
File src/kudu/client/client.h:

PS4, Line 558: Object
KuduTableCreator


http://gerrit.cloudera.org:8080/#/c/3882/4/src/kudu/common/row_operations-test.cc
File src/kudu/common/row_operations-test.cc:

Line 120: enc.Add(RowOperationsPB::UPSERT, row);
hrm, upsert already listed


-- 
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Reorganize range partition client API

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Reorganize range partition client API
..


Patch Set 4:

Build Started http://104.196.14.100/job/kudu-gerrit/2841/

-- 
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Reorganize range partition client API

[Adar Dembo (Code Review)]

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();
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& 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:

[kudu-CR] Reorganize range partition client API

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Reorganize range partition client API
..


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/2797/

-- 
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 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Reorganize range partition client API

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3882

to look at the new patch set (#2).

Change subject: Reorganize range partition client API
..

Reorganize range partition client API

This commit redesigns the client APIs dealing with adding and dropping range
partitions. The goal is to make them more consistent and easier to understand.
The biggest public facing change is that all methods which deal with range
partitions are now named that, and the 'bounds' nomenclature has mostly been
dropped. All of these methods are new since 0.9.1, so this shouldn't be a
breaking change (wire compatibility is not broken).

Additionally, range partitions can now be created, added, and dropped with
exclusive lower bounds and inclusive upper bounds. Originally I thought this
would be a pretty minor feature, but I think it's going to end up being useful
when creating higher level SQL bindings.

An equivalent cleanup for the Java client will be in a follow up commit.

Change-Id: Ic731f0abcbb81cd4238cc960b150bf8cd1c1f0ea
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_alterer-internal.cc
M src/kudu/client/table_alterer-internal.h
M src/kudu/client/table_creator-internal.h
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/wire_protocol.proto
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/flex_partitioning-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/tablet/transactions/write_transaction.cc
17 files changed, 656 insertions(+), 82 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/3882/2
-- 
To view, visit http://gerrit.cloudera.org:8080/3882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic731f0abcbb81cd4238cc960b150bf8cd1c1f0ea
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Reorganize range partition client API

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Reorganize range partition client API
..


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/2794/

-- 
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No