Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17769 )
Change subject: KUDU-2671 key encoding for custom hash bucket schemas ...................................................................... Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/17769/6/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/17769/6/src/kudu/client/client.cc@1016 PS6, Line 1016: if (range->hash_bucket_schemas_.empty()) { : schemas_pb->mutable_hash_schemas()->CopyFrom( : data_->partition_schema_.hash_bucket_schemas()) > Nope, there is no need to require user to manually add a hash dimension for Ah, I guess your point was about the PB structures (which are populated here). If that's what you meant here, I totally agree -- we might optimize this piece, having some sort of agreement how to interpret the elements in the sequence of (range + hash schema) elements. Anyway, I'm planning to update this code, at least to allow for having no hash-based sharding for partitions, when needed. But overall, the internal representation of the range-specific hash schemas and how we populate the PB structures for RPC requests might be different: we should give the client side the ability to be more concise, while we might strive for faster lookup of range-specific schema in the client library. http://gerrit.cloudera.org:8080/#/c/17769/6/src/kudu/client/client.cc@1016 PS6, Line 1016: if (range->hash_bucket_schemas_.empty()) { : schemas_pb->mutable_hash_schemas()->CopyFrom( : data_->partition_schema_.hash_bucket_schemas()) > To be clear, I was referring to when the table-wide hash schema would be ph The Kudu API will be backward-compatible, so the user could still be able to use the KuduTableCreator::add_hash_partitions() call which is setting table-wide schema. So, no: users of the Kudu API will not need to add hash dimensions for each range -- they might opt out to call KuduTableCreator::add_hash_partitions() to specify table-wide schema and then add range partitions with custom hash schemas using KuduRangePartition::add_hash_partitions(). http://gerrit.cloudera.org:8080/#/c/17769/6/src/kudu/client/flex_partitioning_client-test.cc File src/kudu/client/flex_partitioning_client-test.cc: http://gerrit.cloudera.org:8080/#/c/17769/6/src/kudu/client/flex_partitioning_client-test.cc@382 PS6, Line 382: paritions > nit: partitions Done http://gerrit.cloudera.org:8080/#/c/17769/6/src/kudu/client/flex_partitioning_client-test.cc@382 PS6, Line 382: the > remove 'the' Done http://gerrit.cloudera.org:8080/#/c/17769/6/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/17769/6/src/kudu/common/partition.h@499 PS6, Line 499: key > nit: range key Done http://gerrit.cloudera.org:8080/#/c/17769/6/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/17769/6/src/kudu/common/partition.cc@1559 PS6, Line 1559: if (!upper.empty() && upper <= range_key) { : has_custom_range = false; : } > Right, but what will be returned in the case of a non-covered range once th If there is no range for the key, then the method will let the caller know about such a condtition. Of course, it might require changing the function's signature :) -- To view, visit http://gerrit.cloudera.org:8080/17769 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I81aa1c10998d88a1bd5314fc3132037b1c8bbe4b Gerrit-Change-Number: 17769 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Wed, 18 Aug 2021 21:15:48 +0000 Gerrit-HasComments: Yes
