Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17775 )
Change subject: [partition] update naming of related entities ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/17775/1/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/17775/1/src/kudu/common/partition.h@168 PS1, Line 168: hush > nit: hash Done http://gerrit.cloudera.org:8080/#/c/17775/1/src/kudu/common/partition.h@173 PS1, Line 173: HashDimension > How do we reconcile with the fact that within common.proto it's called 'Has Right: I don't think we can to change the naming in the proto files. However, we don't have to strictly mirror the names in here, IMO. Especially given the fact that PartitionSchema isn't a mirror of PartitionSchemaPB, especially after recent updates on the custom hash schema per range partition. With that, I guess my answer is: I don't think we need to reconcile these names to be 1-to-1 between this code and the common.proto. One of the important points for this update is to stop calling a single level/dimension of hash bucketing as 'schema' because it's semantically confusing: a 'schema' means the whole set of rules, not just a part of those. I also added a note to mention the relation with the PB counterpart structure. Do you feel strongly against this approach? http://gerrit.cloudera.org:8080/#/c/17775/1/src/kudu/common/partition.h@175 PS1, Line 175: u > Previously was signed to keep consistent with the protobuf definition of a Good point -- I reverted the change and updated the other similar places. I changed this to be unsigned because by definition the number of buckets cannot be negative. From the other side, I agree that since we already declared num_buckets as signed in proto, we now need to keep it signed and also keep around those funny checks for non-negative numbers everywhere when receiving the number of buckets as input. http://gerrit.cloudera.org:8080/#/c/17775/1/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/17775/1/src/kudu/common/partition.cc@496 PS1, Line 496: lower = std::move(*split); > warning: std::move of the const expression has no effect; remove std::move( Done http://gerrit.cloudera.org:8080/#/c/17775/1/src/kudu/common/partition.cc@602 PS1, Line 602: int index = current_range_hash_schema.empty() ? -1 : i; > warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed Done http://gerrit.cloudera.org:8080/#/c/17775/1/src/kudu/common/partition.cc@602 PS1, Line 602: int index = current_range_hash_schema.empty() ? -1 : i; > warning: narrowing conversion from constant value 18446744073709551615 (0xF Done http://gerrit.cloudera.org:8080/#/c/17775/1/src/kudu/integration-tests/table_locations-itest.cc File src/kudu/integration-tests/table_locations-itest.cc: http://gerrit.cloudera.org:8080/#/c/17775/1/src/kudu/integration-tests/table_locations-itest.cc@487 PS1, Line 487: range_hash_schemas.push_back({}); > warning: use emplace_back instead of push_back [modernize-use-emplace] Done -- To view, visit http://gerrit.cloudera.org:8080/17775 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6a858e97090930b21e9c767dac2f5cc8b9816033 Gerrit-Change-Number: 17775 Gerrit-PatchSet: 1 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-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 16 Aug 2021 23:46:02 +0000 Gerrit-HasComments: Yes
