Mahesh Reddy 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: (7 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()) What's the benefit of storing the table wide hash schema for each range that uses it rather than using an empty index to signify the usage of the table wide hash schema? I guess if eventually if we want to phase out the use of the newly renamed 'hash_schema field' within PartitionSchema then this change makes sense. 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: the remove 'the' http://gerrit.cloudera.org:8080/#/c/17769/6/src/kudu/client/flex_partitioning_client-test.cc@382 PS6, Line 382: paritions nit: partitions 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 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@959 PS6, Line 959: if (key.size() < kEncodedBucketSize * hash_bucket_schemas_.size()) { : return "<hash-decode-error>"; : } Should we check this for per range hash schemas as well? http://gerrit.cloudera.org:8080/#/c/17769/6/src/kudu/common/partition.cc@1558 PS6, Line 1558: // TODO(aserbin): is the upper bound always exclusive? yes, lower bound is always inclusive and upper bound is always exclusive. 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; : } Maybe I'm missing something, but why would this if statement ever be triggered? -- 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: Tue, 17 Aug 2021 21:54:14 +0000 Gerrit-HasComments: Yes
