Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17025 )
Change subject: KUDU-2671: Adds new field to PartitionSchema. ...................................................................... Patch Set 10: (11 comments) http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/client/scan_token-internal.cc@345 PS9, Line 345: &partition_schema_pb) > nit: &partition_schema_pb is an argument of ToPB(), so it should be aligned Done http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/common/partition-test.cc File src/kudu/common/partition-test.cc: PS9: > Could you add some negative tests for malformed partition schemas too? Ack http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/common/partition-test.cc@1300 PS9, Line 1300: pb.add_range_hash_schemas(); > warning: unused variable 'range_hash_component' [clang-diagnostic-unused-va Ack http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/common/partition-test.cc@1303 PS9, Line 1303: PartitionSchema partition_schema; : ASSERT_OK(PartitionSchema::FromPB(pb, schema, &partition_schema)); > nit: would it make sense to define some helper function that takes a Partit hm I could but then there would be less flexibility since the PartitionSchemaPB would have to be the same protobuf since the equality checks on the fields are specific to this protobuf. If that's something we're still interested in I can move a bulk of this code to a helper function. http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/common/partition-test.cc@1347 PS9, Line 1347: ASSERT_TRUE(partition_schema.Equals(partition_schema1)); > nit: use ASSERT_TRUE() for this Done http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/common/partition.cc@204 PS9, Line 204: if (ops.size() != 2) { > In a valid message, do we ever expect there to be more than two ops? If not nope, there should only be two ops, I can modify the code to return an error if ops.size() != 2. I agree that ops[0] should always be the lower bound and ops[1] be the upper bound, but similar protections exist in the CatalogManager where a check exists to verify that ops[1] is indeed the upper bound. I can add the size check for now, we can discuss further about assuming ops[0] is the lower bound and ops[1] is the upper bound. http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/common/partition.cc@209 PS9, Line 209: > The ToPB method seems to only have RANGE_LOWER_BOUND and RANGE_UPPER_BOUND. We would still need the EXCLUSIVE/INCLUSIVE enums in the case that FromPB is decoding a protobuf message from the client. This happens in the catalog_manager in both the create table and alter table paths. http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/common/partition.cc@213 PS9, Line 213: op > nit: add a space Done http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/common/partition.cc@216 PS9, Line 216: : // Lower bound range partition keys are inclusive and upper bound range partition keys : // are exclusive by design. If the provided keys are not of this format, these keys : // will be transformed to their proper format. : if (op1.type == RowOperationsPB::EXCLUSIVE_RANGE_LOWER_BOUND) { : RETURN_NOT_OK(partition_schema->MakeLowerBoundRangePartitionKeyInclusive( : op1.split_row.get())); : } : if > Why don't we need to do anything in the RANGE_LOWER_BOUND or RANGE_UPPER_BO Lower bound range partition keys are inclusive and upper bound range partition keys are exclusive by design, so in the cases that they are not (EXCLUSIVE_RANGE_LOWER_BOUND and INCLUSIVE_RANGE_UPPER_BOUND), some work is required to transform the partition keys. For the RANGE_LOWER_BOUND and RANGE_UPPER_BOUND cases, the keys are already inclusive and exclusive respectively so no extra work is required in these cases. I can add a comment effectively summarizing this. http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/common/partition.cc@249 PS9, Line 249: : if (!range_bounds.empty()) { : RETURN_NOT_OK(partition_schema->EncodeRangeBoun > nit: spacing Done http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/common/partition.cc@274 PS9, Line 274: RowOperationsPBEn > I'm curious if there are performance implications of allocating this repeat Yep I can use the same object and reset it every iteration. -- To view, visit http://gerrit.cloudera.org:8080/17025 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic5d8615ab9967fdb40292b9c77eb68a19baeca1d Gerrit-Change-Number: 17025 Gerrit-PatchSet: 10 Gerrit-Owner: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 31 Mar 2021 01:06:32 +0000 Gerrit-HasComments: Yes
