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: (4 comments) Thank you for the feedback! I'm about to address it in a separate changelist. As of now, I just addressed a few questions. 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 tha Right: eventually there should be no table-wide hash schema stored. There will be a registry of all different hash schemas used by a table's partition schema, and then each range will have a reference (i.e. pointer/index/iterator) pointing to corresponding element in the registry. 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? Good point! I guess this part should have been moved down right before the for() cycle at line 975. 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. Thank you for the confirmation -- I removed this comment in https://gerrit.cloudera.org/#/c/17775/ 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 trigge This statement is triggered in a case of non-covered range: FindFloorOrNull() returns a range with its lower boundary less or equal to the specified range key, but it might happen that even the upper boundary of the range less than the key. -- 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 04:38:37 +0000 Gerrit-HasComments: Yes
