Andrew Wong 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 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/17769/3/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/17769/3/src/kudu/client/client.cc@1017 PS3, Line 1017: nit: got a couple extra spaces here. http://gerrit.cloudera.org:8080/#/c/17769/3/src/kudu/client/flex_partitioning_client-test.cc File src/kudu/client/flex_partitioning_client-test.cc: http://gerrit.cloudera.org:8080/#/c/17769/3/src/kudu/client/flex_partitioning_client-test.cc@328 PS3, Line 328: 44)); : : We may not be able to scan the table yet, but perhaps we can check the table stats and get the number of live rows? http://gerrit.cloudera.org:8080/#/c/17769/3/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/17769/3/src/kudu/common/partition.h@510 PS3, Line 510: // NOTE: this map becomes invalid once 'ranges_with_hash_schemas_' changed nit: rather than mentioning when it's valid, maybe mention that users should clear/set this as ranges_with_hash_schemas_ is cleared and set? I found myself wondering when 'ranges_with_hash_schemas_' could be mutated, and if so, how we ensure this wasn't used. But it seems the usage of these two are fairly tightly coupled. http://gerrit.cloudera.org:8080/#/c/17769/3/src/kudu/common/partition.h@512 PS3, Line 512: hash_schema_ nit: maybe name this and the typedef hash_schema_idx_by... or something, so it's clear at usage sites these refer to an index? http://gerrit.cloudera.org:8080/#/c/17769/3/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/17769/3/src/kudu/common/partition.cc@260 PS3, Line 260: Arena arena(256); : Slice s_lower = Slice(it->lower); : KuduPartialRow lower(&schema); : RETURN_NOT_OK(partition_schema->DecodeRangeKey(&s_lower, &lower, &arena)); I think I'm missing something -- why do we need to decode the range key if we're keying by the encoded range key? http://gerrit.cloudera.org:8080/#/c/17769/3/src/kudu/common/partition.cc@1525 PS3, Line 1525: Part nit: add four spaces since this is a continuation of the previous line? http://gerrit.cloudera.org:8080/#/c/17769/3/src/kudu/common/partition.cc@1528 PS3, Line 1528: hash_schema_by_encoded_range_start_, key If the input 'key' is a partition key, how can we use it as a key to a map that's keyed by range 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: 3 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, 12 Aug 2021 03:35:42 +0000 Gerrit-HasComments: Yes
