Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17025 )
Change subject: KUDU-2671: Adds new field to PartitionSchema. ...................................................................... Patch Set 9: (10 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: nit: &partition_schema_pb is an argument of ToPB(), so it should be aligned as such. 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? 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 PartitionSchemaPB and does this deserialize/serialize/deserialize dance and equality checks? Maybe we could use that in some of the other tests for some extra validation http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/common/partition-test.cc@1347 PS9, Line 1347: ASSERT_EQ(true, partition_schema.Equals(partition_schema1)); nit: use ASSERT_TRUE() for this 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: for (int j = 0; j < ops.size(); j++) { In a valid message, do we ever expect there to be more than two ops? If not, could we return an error if ops.size() isn't 2? If so, could we also assume ops[0] is the RANGE_LOWER_BOUND and ops[1] is the RANGE_UPPER_BOUND? That might make this code block a bit easier to read. http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/common/partition.cc@209 PS9, Line 209: EXCLUSIVE_RANGE_LOWER_BOUND The ToPB method seems to only have RANGE_LOWER_BOUND and RANGE_UPPER_BOUND. Do we actually need the EXCLUSIVE/INCLUSIVE enums here? http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/common/partition.cc@213 PS9, Line 213: nit: add a space http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/common/partition.cc@216 PS9, Line 216: : if (op.type == RowOperationsPB::EXCLUSIVE_RANGE_LOWER_BOUND) { : RETURN_NOT_OK(partition_schema->MakeLowerBoundRangePartitionKeyInclusive( : op.split_row.get())); : } : if (ops[j].type == RowOperationsPB::INCLUSIVE_RANGE_UPPER_BOUND) { : RETURN_NOT_OK(partition_schema->MakeUpperBoundRangePartitionKeyExclusive( : ops[j].split_row.get())); : } Why don't we need to do anything in the RANGE_LOWER_BOUND or RANGE_UPPER_BOUND cases? Could use a comment. http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/common/partition.cc@249 PS9, Line 249: RETURN_NOT_OK( : partition_schema->EncodeRangeBounds(range_bounds, range_hash_schema, schema, : &partit nit: spacing http://gerrit.cloudera.org:8080/#/c/17025/9/src/kudu/common/partition.cc@274 PS9, Line 274: Arena arena(256); I'm curious if there are performance implications of allocating this repeatedly. Is it possible to / would it make sense to reset and reuse the same arena? -- 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: 9 Gerrit-Owner: Mahesh Reddy <[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: Tue, 30 Mar 2021 07:29:03 +0000 Gerrit-HasComments: Yes
