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 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. Done 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 tabl Good point. Done. 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 shoul Done 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 Done 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 Ah, good catch -- not sure how I missed this. That must be a result of conflict resolution or something while squashing commits and re-basing: this piece doesn't effectively do anything to affect it->lower, so it's a no-op :) 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? Well, instead I suggest to treat the previous line same as if it were template<typename T> Just discovered that making those continuations between the return type and the method name look way off and bring more puzzle than readability :) I can change this if you feel strongly about changing the formatting as suggested, though. 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 You are right: the comment for the method is incorrect -- that's the range key, of course. Here I used 'partition key' notion in meaning of ranges in one linear dimension. I fixed this terminology issue. -- 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: 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 08:54:08 +0000 Gerrit-HasComments: Yes
