Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17890 )
Change subject: KUDU-2671 introduce PartitionKey ...................................................................... Patch Set 7: (4 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_par Right: the hash key, if present, is set either in the lower or the upper bound. 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? I guess I'm going to drop this in the follow-up changelist -- it seems that's the only comment that needs some addressing, so I guess it's easier to do so in next iteration. http://gerrit.cloudera.org:8080/#/c/17890/7/src/kudu/common/partition.cc@628 PS7, Line 628: << p.begin_.DebugString(); > Is this still necessary? It's not exactly necessary, but it might help to see what's the PartitionKey if this ever fails -- I added this when addressing Andrew's feedback. 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 P Yes, that's possible: * the range part is empty, but the hash part is non-empty -- that's for unbounded range with some hash bucketing * the range part is non-empty, but the hash part is empty -- that the case of a range with exact lower bound, but with no hash bucketing -- 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: Fri, 08 Oct 2021 00:46:53 +0000 Gerrit-HasComments: Yes
