Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16596 )
Change subject: [partitioning] KUDU-2671: Support for range specific HashSchemas. ...................................................................... Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/16596/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16596/6//COMMIT_MSG@18 PS6, Line 18: n specify ambiguous http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition-test.cc File src/kudu/common/partition-test.cc: http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition-test.cc@1000 PS8, Line 1000: { // [(a1, _, c1), (a2, _, c2)) : KuduPartialRow lower(&schema); : KuduPartialRow upper(&schema); : ASSERT_OK(lower.SetStringCopy("a", "a1")); : ASSERT_OK(lower.SetStringCopy("c", "c1")); : ASSERT_OK(upper.SetStringCopy("a", "a2")); : ASSERT_OK(upper.SetStringCopy("c", "c2")); : PartitionSchema::HashBucketSchemas hash_schema_4_buckets = {{{ColumnId(0)}, 4, 0}}; : bounds.emplace_back(lower, upper); : range_hash_schemas.emplace_back(hash_schema_4_buckets); : bounds_with_hash_schemas.emplace_back(make_pair(std::move(lower), std::move(upper)), : std::move(hash_schema_4_buckets)); : } Would it make sense to check for the result of partition building after each of these smaller blocks? http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@128 PS8, Line 128: column_ids nit: not exactly the essential part of this changelist, but since you are touching this code anyways, consider making few improvements here. Maybe, it's more convenient to do so in a separate changelist. Since this method can return non-OK status when an error is detected in the middle, consider not changing the output parameter in the middle, but create a local variable and do std::move() in the end before returning Status::OK(): vector<ColumnId> col_ids; col_ids.reserve(identifiers.size()); ... *column_ids = std::move(col_ids); Also, move the 'continue' to the same scope as their corresponding 'case' label is. http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@409 PS8, Line 409: return Status::InvalidArgument("Both 'split_rows' and 'range_hash_schemas' cannot be " : "populated at the same time."); Could you add a testcase to trigger this error to make sure this method behaves as expected? -- To view, visit http://gerrit.cloudera.org:8080/16596 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8725f4bd072a81b05b36dfc7df0c074c172b4ce8 Gerrit-Change-Number: 16596 Gerrit-PatchSet: 8 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 25 Nov 2020 01:27:39 +0000 Gerrit-HasComments: Yes
