Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17657 )
Change subject: WIP [client] KUDU-2671 flexible partitioning during table creation ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/17657/1/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/17657/1/src/kudu/client/client-test.cc@6895 PS1, Line 6895: This effectively : // cancels the multi-level partitioning defined by the table-wide hash : // partition schema, so the newly added range has no hash sub-partitioning. Maybe I'm missing it, but isn't this equivalent to calling add_range_partition()? In both cases, wouldn't we add a KuduRangePartition that has an empty list of hash bucket schemas? In which case, wouldn't this be using the table-wide schema as well? http://gerrit.cloudera.org:8080/#/c/17657/1/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/17657/1/src/kudu/client/client.h@1214 PS1, Line 1214: add_hash_partitioning nit: maybe call it 'add_hash_partitions()' for consistency with the TableCreator API? http://gerrit.cloudera.org:8080/#/c/17657/1/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/17657/1/src/kudu/client/client.cc@869 PS1, Line 869: add_range_partition_custom_hash_buckets nit: I wonder if it makes sense to call this add_custom_range_partition() or somesuch, removing the "hash partition" specification. I can imagine KuduRangePartition growing into something more widely used (e.g. to supply tags for a newly provisioned range). http://gerrit.cloudera.org:8080/#/c/17657/1/src/kudu/client/table_creator-internal.cc File src/kudu/client/table_creator-internal.cc: http://gerrit.cloudera.org:8080/#/c/17657/1/src/kudu/client/table_creator-internal.cc@58 PS1, Line 58: // TODO(aserbin): any restrictions on the value of the 'seed' parameter? It's a good question -- my gut says no to be consistent with the table-wide add_hash_partitions(), but I'm not sure. http://gerrit.cloudera.org:8080/#/c/17657/1/src/kudu/client/table_creator-internal.cc@60 PS1, Line 60: le nit: typo -- To view, visit http://gerrit.cloudera.org:8080/17657 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I98fd9754db850dcdd00a00738f470673f42ac5b4 Gerrit-Change-Number: 17657 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Thu, 08 Jul 2021 20:01:40 +0000 Gerrit-HasComments: Yes
