Dan Burkert has posted comments on this change.

Change subject: Reorganize range partition client API
......................................................................


Patch Set 4:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/3882/3/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

Line 1288:   }
> Use an ElementDeleter instead, and declare it just after 'errors'.
Done


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

Line 541:   ///   The type of the lower bound, either inclusive or exclusive. 
Defaults to
> Should add @param entries for the bound types.
Done


Line 808:   ///   logical minimum value for the column's type will be used by 
default.
> Should add @param entries for the bound types.
Done


Line 835:   ///   alterer.
> Should add @param entries for the bound types.
Done


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

Line 657: 
> Could you precondition up front that 'test' has the expected number of elem
Done


http://gerrit.cloudera.org:8080/#/c/3882/3/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:

Line 921:       return Status::InvalidArgument("must specify existing columns 
for range "
> What about STRING and TIMESTAMP?
Added those (and a test case that would have failed). Correct about 
bool/float/double.


http://gerrit.cloudera.org:8080/#/c/3882/3/src/kudu/common/partition.h
File src/kudu/common/partition.h:

PS3, Line 198:   // Returns a text description of this partition schema 
suitable for display in the web UI.
             :   // The format of this string is not guarantee
> And here I think you meant 'inclusive' first. And there's an extra 'in'.
Done


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 rele
They have not been released, but they follow the convention that lower bounds 
are always inclusive, and upper bounds are always exclusive unless otherwise 
marked.


Line 164:     // Should be followed by the associated upper bound. If all 
values are
> Nit: "If all values are missing..."
Done


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:     if (ts_.rand_.OneIn(2) && lower_bound_value > INT32_MIN) {
> Can we get some coverage here and below of the different exclusive/inclusiv
Done


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.
Done


-- 
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 <d...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to