Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17890 )
Change subject: KUDU-2671 introduce PartitionKey ...................................................................... Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/17890/7/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: http://gerrit.cloudera.org:8080/#/c/17890/7/src/kudu/client/meta_cache.cc@387 PS7, Line 387: const auto& entry_hash_key = !lower_bound_partition_key().empty() Maybe I'm missing something, but why do we not check if the upper_bound_partition_key is empty? Is one of the lower or upper bound partition keys guaranteed to be set? http://gerrit.cloudera.org:8080/#/c/17890/7/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/17890/7/src/kudu/common/partition.cc@108 PS7, Line 108: // Check the invariant: valid partition cannot span over multiple sets Since this looks incomplete, maybe add a TODO here? http://gerrit.cloudera.org:8080/#/c/17890/7/src/kudu/common/partition.cc@628 PS7, Line 628: << p.begin_.DebugString(); Is this still necessary? http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/master/catalog_manager.cc@6394 PS4, Line 6394: , > Well, this is shell-like notation (particularly, Bourne shell). Adding a s Nope I don't feel strongly about it, we can keep it as is. http://gerrit.cloudera.org:8080/#/c/17890/7/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17890/7/src/kudu/master/catalog_manager.cc@6399 PS7, Line 6399: start.has_hash_key() || start.has_range_key()) What if only one of hash or range key is set? Would the other part of the PartitionKey just be empty? -- To view, visit http://gerrit.cloudera.org:8080/17890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I00255ec404beeb999117f5265de0d5d8deaf0d68 Gerrit-Change-Number: 17890 Gerrit-PatchSet: 7 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: Thu, 07 Oct 2021 22:38:03 +0000 Gerrit-HasComments: Yes
